Update plugin system and implement aleternative home for LS3 (Insee) …#1078
Open
garronej wants to merge 13 commits into
Open
Update plugin system and implement aleternative home for LS3 (Insee) …#1078garronej wants to merge 13 commits into
garronej wants to merge 13 commits into
Conversation
…as an example usecase
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a new plugin system ( ChangesPlugin system and HomeLS3 IDE launcher
Env variable removal
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
participant main as main.lazy.tsx
participant initPluginSystem as initPluginSystem()
participant window as window.onOnyxiaCtxReady
end
rect rgba(255, 222, 173, 0.5)
participant Plugin as External Plugin
participant evtDeclaredComponents as evtDeclaredComponents
participant App as App.tsx
end
main->>initPluginSystem: call at startup
initPluginSystem->>window: invoke existing callback with onyxiaCtx
initPluginSystem->>window: install Object.defineProperty setter
Plugin->>window: assign window.onOnyxiaCtxReady = cb
window->>Plugin: cb(onyxiaCtx) immediately
Plugin->>evtDeclaredComponents: onyxiaCtx.declareComponent(Component).mount(el)
evtDeclaredComponents-->>App: state change via useRerenderOnStateChange
App->>App: createPortal(Component, containerElement)
sequenceDiagram
rect rgba(200, 230, 200, 0.5)
participant User
participant HomeLS3
end
rect rgba(255, 222, 173, 0.5)
participant HomeLS3GitDialog
participant routes as routes.launcher
end
User->>HomeLS3: click service card
HomeLS3->>HomeLS3GitDialog: evtOpen fires with serviceName + onUserResponse
alt cancel
HomeLS3GitDialog-->>HomeLS3: response = cancel (Deferred stays pending)
else launch without git repo
HomeLS3GitDialog-->>HomeLS3: response = launch without git repo
HomeLS3->>routes: launcher(catalogId: ide, chartName)
else launch with git repo
HomeLS3GitDialog-->>HomeLS3: response = launch with git repo, gitlabRepoUrl
HomeLS3->>routes: launcher(catalogId: ide, chartName, helmValuesPatch: git.repository)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web/src/pluginSystem/pluginSystem.ts (1)
92-94: ⚡ Quick winValidate container before mounting.
The
containerparameter is accepted without validation. If a plugin passes an invalid or detached DOM element,createPortalinApp.tsxwill fail silently or throw at render time.🛡️ Proposed validation
mountComponent: ({ Component, container }) => { + if (!(container instanceof HTMLElement)) { + throw new TypeError("container must be an HTMLElement"); + } + if (!container.isConnected) { + console.warn("Mounting to a detached container; portal may not render."); + } evtPluginComponent.state = { Component, container }; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/pluginSystem.ts` around lines 92 - 94, Validate the plugin mount target before assigning evtPluginComponent.state in mountComponent: ensure the passed container is a DOM Element (container instanceof Element) and is attached to the document (document.contains(container)); if the check fails, do not set evtPluginComponent.state and instead surface an error (e.g., console.error or existing logger) or throw so the failure is explicit (this affects mountComponent, evtPluginComponent.state, Component and the createPortal usage in App.tsx).web/public/custom-resources-example/ls3/HomeLS3.js (1)
21-28: ⚡ Quick winPass children as third argument to React.createElement.
Per React conventions, children should be passed as the third argument to
React.createElement(type, props, ...children)rather than inside the props object. While both approaches work, the idiomatic pattern is clearer and matches React's API design.♻️ Proposed refactor
return React.createElement( Text, { typo: "object heading", - className: classes.root, - children: "My Alternative Home With Onyxia-ui Text" + className: classes.root }, + "My Alternative Home With Onyxia-ui Text" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/public/custom-resources-example/ls3/HomeLS3.js` around lines 21 - 28, The return in HomeLS3.js creates a Text element with children placed inside the props object; update the React.createElement call to follow the idiomatic signature by removing the children property from the props object and passing "My Alternative Home With Onyxia-ui Text" as the third argument to React.createElement(Text, { typo: "object heading", className: classes.root }, "My Alternative Home With Onyxia-ui Text").web/public/custom-resources-example/ls3/main.js (1)
1-1: ⚡ Quick winRemove unused import and commented code.
The
createHomeLS3import (line 1) is unused, and the commented-out alternative implementation (line 10) should be removed. This example demonstrates the plugin system usingonyxia.import("ui/shared/HomeLS3"), so keeping the commented alternative implementation adds confusion rather than clarity.♻️ Proposed cleanup
-import { createHomeLS3 } from "./HomeLS3.js"; import { registerPageContainerListener } from "./registerPageContainerListener.js"; /** `@typedef` {import("../../../src/pluginSystem").Onyxia} Onyxia */ /** `@param` {Onyxia} onyxia */ export async function main(onyxia) { const { HomeLS3 } = await onyxia.import("ui/shared/HomeLS3"); - //const { HomeLS3 } = await createHomeLS3(onyxia); console.log("===>", import.meta.url);Also applies to: 10-10
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/public/custom-resources-example/ls3/main.js` at line 1, Remove the unused import createHomeLS3 and the commented-out alternative implementation to avoid confusion; specifically delete the import statement referencing createHomeLS3 and remove the commented code that shows an alternative implementation (the block that suggests using createHomeLS3 instead of onyxia.import("ui/shared/HomeLS3")), leaving only the example that demonstrates onyxia.import("ui/shared/HomeLS3") so the plugin usage is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/public/custom-resources-example/ls3/registerPageContainerListener.js`:
- Around line 6-34: The registerPageContainerListener function creates a
MutationObserver and never disconnects it; modify registerPageContainerListener
to return a cleanup function that disconnects the observer and clears
element_cache so observers don't accumulate. Locate
registerPageContainerListener, the local variables element_cache and observer,
and the update() closure; after observer.observe(...) add a returned function
like () => { observer.disconnect(); element_cache = undefined; } and ensure
callers invoke that cleanup on unload or when replacing listeners.
In `@web/src/ui/shared/HomeLS3.tsx`:
- Line 61: Update the French text string "Pour integrer GIT, lire ça:" in
HomeLS3.tsx to use the correct accent: change "integrer" to "intégrer" so it
reads "Pour intégrer GIT, lire ça:"; locate the literal string in the HomeLS3
component (the JSX/return text) and replace it accordingly.
- Line 43: The greeting string in the Text component in HomeLS3 (Text with prop
typo="page heading") uses the misspelled "Bienvenu"; change it to the correct
French "Bienvenue" so the rendered greeting reads "Bienvenue {user.firstName}".
Ensure you only update the string literal inside the Text component (leave the
Text component, typo prop, and user.firstName interpolation unchanged).
- Around line 95-103: The helmValuesPatch is incorrectly using a hardcoded URL
instead of the user's input: replace the hardcoded string
"https://github.com/InseeFrLab/onyxia" with the gitRepositoryUrl variable so the
patch value uses the user-provided repository; keep the existing ternary that
sets helmValuesPatch to undefined when gitRepositoryUrl is undefined and ensure
the patch entry still targets path ["git","repository"] (i.e., value:
gitRepositoryUrl).
---
Nitpick comments:
In `@web/public/custom-resources-example/ls3/HomeLS3.js`:
- Around line 21-28: The return in HomeLS3.js creates a Text element with
children placed inside the props object; update the React.createElement call to
follow the idiomatic signature by removing the children property from the props
object and passing "My Alternative Home With Onyxia-ui Text" as the third
argument to React.createElement(Text, { typo: "object heading", className:
classes.root }, "My Alternative Home With Onyxia-ui Text").
In `@web/public/custom-resources-example/ls3/main.js`:
- Line 1: Remove the unused import createHomeLS3 and the commented-out
alternative implementation to avoid confusion; specifically delete the import
statement referencing createHomeLS3 and remove the commented code that shows an
alternative implementation (the block that suggests using createHomeLS3 instead
of onyxia.import("ui/shared/HomeLS3")), leaving only the example that
demonstrates onyxia.import("ui/shared/HomeLS3") so the plugin usage is clear.
In `@web/src/pluginSystem/pluginSystem.ts`:
- Around line 92-94: Validate the plugin mount target before assigning
evtPluginComponent.state in mountComponent: ensure the passed container is a DOM
Element (container instanceof Element) and is attached to the document
(document.contains(container)); if the check fails, do not set
evtPluginComponent.state and instead surface an error (e.g., console.error or
existing logger) or throw so the failure is explicit (this affects
mountComponent, evtPluginComponent.state, Component and the createPortal usage
in App.tsx).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b2f9679-3226-4a00-9de9-6c37397f6f74
📒 Files selected for processing (15)
web/public/custom-resources-example/jsconfig.jsonweb/public/custom-resources-example/ls3/HomeLS3.jsweb/public/custom-resources-example/ls3/git-tutorial.mdweb/public/custom-resources-example/ls3/main.jsweb/public/custom-resources-example/ls3/registerPageContainerListener.jsweb/public/custom-resources-example/main.jsweb/public/custom-resources/.gitignoreweb/public/custom-resources/my-plugin.jsweb/public/custom-resources/my-plugin.tsweb/src/pluginSystem/index.tsweb/src/pluginSystem/onyxia_import.tsweb/src/pluginSystem/pluginSystem.tsweb/src/ui/App/App.tsxweb/src/ui/App/Main.tsxweb/src/ui/shared/HomeLS3.tsx
💤 Files with no reviewable changes (2)
- web/public/custom-resources/my-plugin.ts
- web/public/custom-resources/my-plugin.js
| return ( | ||
| <div className={classes.root}> | ||
| {user !== undefined && ( | ||
| <Text typo="page heading">Bienvenu {user.firstName}</Text> |
Contributor
There was a problem hiding this comment.
French spelling error in greeting.
"Bienvenu" should be "Bienvenue" (with final 'e') for proper French grammar in a general welcoming context.
📝 Proposed fix
- <Text typo="page heading">Bienvenu {user.firstName}</Text>
+ <Text typo="page heading">Bienvenue {user.firstName}</Text>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Text typo="page heading">Bienvenu {user.firstName}</Text> | |
| <Text typo="page heading">Bienvenue {user.firstName}</Text> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3.tsx` at line 43, The greeting string in the Text
component in HomeLS3 (Text with prop typo="page heading") uses the misspelled
"Bienvenu"; change it to the correct French "Bienvenue" so the rendered greeting
reads "Bienvenue {user.firstName}". Ensure you only update the string literal
inside the Text component (leave the Text component, typo prop, and
user.firstName interpolation unchanged).
Comment on lines
+95
to
+103
| helmValuesPatch: | ||
| gitRepositoryUrl === undefined | ||
| ? undefined | ||
| : [ | ||
| { | ||
| path: ["git", "repository"], | ||
| value: "https://github.com/InseeFrLab/onyxia" | ||
| } | ||
| ], |
Contributor
There was a problem hiding this comment.
Critical bug: Hardcoded repository URL ignores user input.
The helmValuesPatch uses a hardcoded repository URL "https://github.com/InseeFrLab/onyxia" instead of the gitRepositoryUrl state variable that the user configures via the TextField above. This means the user's Git repository input is collected but completely ignored during service launch.
🐛 Proposed fix
helmValuesPatch:
gitRepositoryUrl === undefined
? undefined
: [
{
path: ["git", "repository"],
- value: "https://github.com/InseeFrLab/onyxia"
+ value: gitRepositoryUrl
}
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| helmValuesPatch: | |
| gitRepositoryUrl === undefined | |
| ? undefined | |
| : [ | |
| { | |
| path: ["git", "repository"], | |
| value: "https://github.com/InseeFrLab/onyxia" | |
| } | |
| ], | |
| helmValuesPatch: | |
| gitRepositoryUrl === undefined | |
| ? undefined | |
| : [ | |
| { | |
| path: ["git", "repository"], | |
| value: gitRepositoryUrl | |
| } | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3.tsx` around lines 95 - 103, The helmValuesPatch is
incorrectly using a hardcoded URL instead of the user's input: replace the
hardcoded string "https://github.com/InseeFrLab/onyxia" with the
gitRepositoryUrl variable so the patch value uses the user-provided repository;
keep the existing ternary that sets helmValuesPatch to undefined when
gitRepositoryUrl is undefined and ensure the patch entry still targets path
["git","repository"] (i.e., value: gitRepositoryUrl).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx (1)
24-26: 💤 Low valueConsider removing or populating empty style object.
The
rootstyle object is empty and serves no purpose. Either remove it if no styles are needed, or populate it with actual styling rules.♻️ Proposed options
Option 1: Remove if unused
-const useStyles = tss.withName({ HomeLS3ServiceCard }).create(() => ({ - root: {} -}));Option 2: Add actual styles if needed
const useStyles = tss.withName({ HomeLS3ServiceCard }).create(() => ({ - root: {} + root: { + // Add styles as needed + } }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx` around lines 24 - 26, The styles object created by useStyles (tss.withName({ HomeLS3ServiceCard }).create) contains an unused empty "root" entry; either remove the "root" property and any references to it in the HomeLS3ServiceCard component, or populate "root" with the needed CSS rules (e.g., layout/padding/typography) and keep useStyles as-is — locate the useStyles declaration and the HomeLS3ServiceCard component to apply the chosen change.web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx (1)
182-208: 💤 Low valueRemove redundant React fragment.
The fragment wrapping the GitLab repo URL
TextField(lines 182-208) contains only a single child and is redundant.♻️ Proposed fix
) : ( - <> <TextField label="URL du dépot GitLab" helperText={<>URL du repo gitlab a cloner dans le service</>} doOnlyShowErrorAfterFirstFocusLost={false} defaultValue={gitlabRepoUrl ?? ""} evtAction={evtTextFieldAction} getIsValidValue={token => { if (token !== "" && !GITLAB_REPO_REGEXP.test(token)) { return { isValidValue: false, message: <>Pas un url GitLab valide</> }; } return { isValidValue: true }; }} onValueBeingTypedChange={params => { setIsValidateTokenDisabled(!params.isValidValue); if (params.isValidValue) { setGitlabRepoUrl(params.value); } }} /> - </> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx` around lines 182 - 208, The JSX contains a redundant React fragment wrapping a single TextField in the HomeLS3GitDialog component; remove the surrounding fragment so the TextField is returned directly (keep all TextField props and handlers like label, helperText, defaultValue, evtAction, getIsValidValue, onValueBeingTypedChange and state setters such as setGitlabRepoUrl/setIsValidateTokenDisabled unchanged) so the component JSX remains valid without the unnecessary <>...</>.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/ui/shared/HomeLS3/HomeLS3.tsx`:
- Around line 62-70: The helmValuesPatch currently hardcodes the git.repository
to "https://github.com/InseeFrLab/onyxia" instead of using the user-provided
gitlabRepoUrl; update the helmValuesPatch construction (the object keyed by
helmValuesPatch in HomeLS3.tsx) so that when gitlabRepoUrl is defined you set
the patch entry's value to gitlabRepoUrl (not the hardcoded URL), preserving the
same path ["git","repository"] shape and keeping the undefined branch when
gitlabRepoUrl is undefined; make sure you reference the gitlabRepoUrl variable
in the value field so the user's input is applied.
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx`:
- Line 101: The code is storing a GitLab Personal Access Token under the
misleading key githubPersonalAccessToken; rename this key and all usages to
gitlabPersonalAccessToken to avoid cross-service credential collisions. Update
the destructuring from useCoreState (const { githubPersonalAccessToken } =
useCoreState(...)) to use gitlabPersonalAccessToken, and update every occurrence
inside HomeLS3GitDialog (including the handlers and JSX references around the
referenced lines ~114, ~120, and ~145-147) so the state read, write, validation,
and any prop names consistently use gitlabPersonalAccessToken; ensure any
set/update calls and persisted storage keys are changed to match.
- Around line 156-158: The validation error message in HomeLS3GitDialog's JSX
currently shows the incorrect GitLab token prefix "gplat-xxx"; update that
string to "glpat-xxx" so the message correctly reflects the GitLab token format.
Locate the token validation/label text inside the HomeLS3GitDialog component
(the JSX fragment showing "Un Token GitLab doit être de la forme gplat-xxx") and
replace the literal with "glpat-xxx" to fix the typo.
In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx`:
- Line 17: The img in HomeLS3Hero.tsx (<img
src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} />) is
missing an alt attribute; add an appropriate alt string (e.g., "LS3 logo" or a
more descriptive phrase relevant to the site) to the <img> element in the
HomeLS3Hero component so screen readers can describe the image.
- Around line 19-23: Update the user-facing French copy in the HomeLS3Hero
component: change the heading Text (Text typo="object heading") from "Bienvenu
{userDisplayName}" to "Bienvenue {userDisplayName}" and update the subtitle Text
(Text typo="subtitle") to "Démarre ton service en quelques clics et profite de
la puissance de calcul de nos serveurs." to fix spelling, accents, plural
agreement, and noun form.
In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx`:
- Line 19: Update the button label in the HomeLS3ServiceCard component to fix
the French spelling: replace the current text "Demarer un {serviceName}" in the
Button JSX (the Button with onClick={onClick}) with the correct "Démarrer un
{serviceName}" so the accent and spelling are correct while keeping the
serviceName interpolation and onClick handler unchanged.
- Line 18: The <img> in the HomeLS3ServiceCard component is missing an alt
attribute; update the img element in HomeLS3ServiceCard.tsx (the element using
coverImageUrl) to include a descriptive alt (preferably from an existing prop
like service title or a coverImageAlt prop) and fall back to an empty alt="" if
the image is purely decorative so screen readers are handled properly.
---
Nitpick comments:
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx`:
- Around line 182-208: The JSX contains a redundant React fragment wrapping a
single TextField in the HomeLS3GitDialog component; remove the surrounding
fragment so the TextField is returned directly (keep all TextField props and
handlers like label, helperText, defaultValue, evtAction, getIsValidValue,
onValueBeingTypedChange and state setters such as
setGitlabRepoUrl/setIsValidateTokenDisabled unchanged) so the component JSX
remains valid without the unnecessary <>...</>.
In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx`:
- Around line 24-26: The styles object created by useStyles (tss.withName({
HomeLS3ServiceCard }).create) contains an unused empty "root" entry; either
remove the "root" property and any references to it in the HomeLS3ServiceCard
component, or populate "root" with the needed CSS rules (e.g.,
layout/padding/typography) and keep useStyles as-is — locate the useStyles
declaration and the HomeLS3ServiceCard component to apply the chosen change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dee1105c-ec7b-4fca-98d4-4e8a85aede5d
⛔ Files ignored due to path filters (3)
web/public/custom-resources-example/ls3/assets/Jupyter.pngis excluded by!**/*.pngweb/public/custom-resources-example/ls3/assets/RStudio.jpgis excluded by!**/*.jpgweb/public/custom-resources-example/ls3/assets/ls3-logo.pngis excluded by!**/*.png
📒 Files selected for processing (8)
web/public/custom-resources-example/ls3/assets/VSCode.webpweb/public/custom-resources-example/ls3/how-to-get-my-gitlab-token.mdweb/src/pluginSystem/onyxia_import.tsweb/src/ui/shared/HomeLS3/HomeLS3.tsxweb/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsxweb/src/ui/shared/HomeLS3/HomeLS3Hero.tsxweb/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsxweb/src/ui/shared/HomeLS3/index.ts
💤 Files with no reviewable changes (1)
- web/public/custom-resources-example/ls3/how-to-get-my-gitlab-token.md
✅ Files skipped from review due to trivial changes (1)
- web/src/ui/shared/HomeLS3/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pluginSystem/onyxia_import.ts
| }) { | ||
| const { serviceName, onProceed } = props; | ||
|
|
||
| const { githubPersonalAccessToken } = useCoreState("userConfigs", "userConfigs"); |
Contributor
There was a problem hiding this comment.
Major data integrity issue: GitLab token stored with GitHub key name.
The code uses the key "githubPersonalAccessToken" to store and retrieve what is actually a GitLab Personal Access Token. This naming inconsistency creates serious confusion and could lead to security issues where GitHub and GitLab credentials are mixed up or overwrite each other.
The key should be renamed to accurately reflect that it stores a GitLab token, not a GitHub token.
♻️ Proposed fix
Update the key name throughout the codebase:
- const { githubPersonalAccessToken } = useCoreState("userConfigs", "userConfigs");
+ const { gitlabPersonalAccessToken } = useCoreState("userConfigs", "userConfigs"); onSubmit={token => {
userConfigs.changeValue({
- key: "githubPersonalAccessToken",
+ key: "gitlabPersonalAccessToken",
value: token
});
}}And update line 114:
- const doInjectGitUrl = !!githubPersonalAccessToken && !!gitlabRepoUrl;
+ const doInjectGitUrl = !!gitlabPersonalAccessToken && !!gitlabRepoUrl;And line 120:
- {!githubPersonalAccessToken ? (
+ {!gitlabPersonalAccessToken ? (Also applies to: 145-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx` at line 101, The code is
storing a GitLab Personal Access Token under the misleading key
githubPersonalAccessToken; rename this key and all usages to
gitlabPersonalAccessToken to avoid cross-service credential collisions. Update
the destructuring from useCoreState (const { githubPersonalAccessToken } =
useCoreState(...)) to use gitlabPersonalAccessToken, and update every occurrence
inside HomeLS3GitDialog (including the handlers and JSX references around the
referenced lines ~114, ~120, and ~145-147) so the state read, write, validation,
and any prop names consistently use gitlabPersonalAccessToken; ensure any
set/update calls and persisted storage keys are changed to match.
|
|
||
| return ( | ||
| <div className={cx(classes.root, className)}> | ||
| <img src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} /> |
Contributor
There was a problem hiding this comment.
Missing alt attribute on image.
The <img> element lacks an alt attribute, which is required for accessibility. Screen readers need this to describe the image to visually impaired users.
♿ Proposed fix
- <img src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} />
+ <img
+ src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`}
+ alt="Logo LS3"
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <img src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} /> | |
| <img | |
| src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} | |
| alt="Logo LS3" | |
| /> |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 17-17: img elements must have an alt prop, either with meaningful text, or an empty string for decorative images.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx` at line 17, The img in
HomeLS3Hero.tsx (<img
src={`${PUBLIC_URL}custom-resources-example/ls3/assets/ls3-logo.png`} />) is
missing an alt attribute; add an appropriate alt string (e.g., "LS3 logo" or a
more descriptive phrase relevant to the site) to the <img> element in the
HomeLS3Hero component so screen readers can describe the image.
Comment on lines
+19
to
+23
| <Text typo="object heading">Bienvenu {userDisplayName}</Text> | ||
| <Text typo="subtitle"> | ||
| Demare ton service en quelque clicks et profite de la puissance de | ||
| calcule de nos serveurs. | ||
| </Text> |
Contributor
There was a problem hiding this comment.
French grammar and spelling errors in user-facing text.
The text contains several French language errors:
- Line 19: "Bienvenu" → should be "Bienvenue" (feminine/neutral form)
- Line 21: "Demare" → should be "Démarre" (correct spelling with accent)
- Line 21: "quelque clicks" → should be "quelques clics" (plural agreement and French word)
- Line 22: "calcule" → should be "calcul" (noun form)
📝 Proposed fix
- <Text typo="object heading">Bienvenu {userDisplayName}</Text>
+ <Text typo="object heading">Bienvenue {userDisplayName}</Text>
<Text typo="subtitle">
- Demare ton service en quelque clicks et profite de la puissance de
- calcule de nos serveurs.
+ Démarre ton service en quelques clics et profite de la puissance de
+ calcul de nos serveurs.
</Text>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Text typo="object heading">Bienvenu {userDisplayName}</Text> | |
| <Text typo="subtitle"> | |
| Demare ton service en quelque clicks et profite de la puissance de | |
| calcule de nos serveurs. | |
| </Text> | |
| <Text typo="object heading">Bienvenue {userDisplayName}</Text> | |
| <Text typo="subtitle"> | |
| Démarre ton service en quelques clics et profite de la puissance de | |
| calcul de nos serveurs. | |
| </Text> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx` around lines 19 - 23, Update the
user-facing French copy in the HomeLS3Hero component: change the heading Text
(Text typo="object heading") from "Bienvenu {userDisplayName}" to "Bienvenue
{userDisplayName}" and update the subtitle Text (Text typo="subtitle") to
"Démarre ton service en quelques clics et profite de la puissance de calcul de
nos serveurs." to fix spelling, accents, plural agreement, and noun form.
|
|
||
| return ( | ||
| <div className={cx(classes.root, className)}> | ||
| <img src={coverImageUrl} /> |
Contributor
There was a problem hiding this comment.
Missing alt attribute on image.
The <img> element lacks an alt attribute, which is required for accessibility. Screen readers need this to describe the service image to visually impaired users.
♿ Proposed fix
- <img src={coverImageUrl} />
+ <img src={coverImageUrl} alt={`${serviceName} service`} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <img src={coverImageUrl} /> | |
| <img src={coverImageUrl} alt={`${serviceName} service`} /> |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 18-18: img elements must have an alt prop, either with meaningful text, or an empty string for decorative images.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx` at line 18, The <img> in
the HomeLS3ServiceCard component is missing an alt attribute; update the img
element in HomeLS3ServiceCard.tsx (the element using coverImageUrl) to include a
descriptive alt (preferably from an existing prop like service title or a
coverImageAlt prop) and fall back to an empty alt="" if the image is purely
decorative so screen readers are handled properly.
| return ( | ||
| <div className={cx(classes.root, className)}> | ||
| <img src={coverImageUrl} /> | ||
| <Button onClick={onClick}>Demarer un {serviceName}</Button> |
Contributor
There was a problem hiding this comment.
French spelling error in button text.
"Demarer" should be "Démarrer" (with accent on the first 'e' and double 'r').
📝 Proposed fix
- <Button onClick={onClick}>Demarer un {serviceName}</Button>
+ <Button onClick={onClick}>Démarrer un {serviceName}</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button onClick={onClick}>Demarer un {serviceName}</Button> | |
| <Button onClick={onClick}>Démarrer un {serviceName}</Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx` at line 19, Update the
button label in the HomeLS3ServiceCard component to fix the French spelling:
replace the current text "Demarer un {serviceName}" in the Button JSX (the
Button with onClick={onClick}) with the correct "Démarrer un {serviceName}" so
the accent and spelling are correct while keeping the serviceName interpolation
and onClick handler unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog_backup.tsx`:
- Line 101: The HomeLS3GitDialog_backup component is incorrectly using
`githubPersonalAccessToken` (a GitHub-specific key) to store and retrieve GitLab
credentials, which violates key semantics and risks colliding with actual GitHub
tokens in shared user config state. Replace the `githubPersonalAccessToken` key
with a GitLab-specific key name (such as `gitlabPersonalAccessToken`) in the
useCoreState call and in all other locations where this token is accessed or
stored within the dialog component to ensure proper semantic separation and
prevent credential collisions.
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx`:
- Around line 127-137: The isRepoUrlValid state is initialized to true on the
first render, but if there is a persisted invalid gitlabRepoUrl value, the
doInjectGitUrl condition would still evaluate to true and allow submitting the
malformed URL without user interaction. Initialize isRepoUrlValid based on the
actual validity of the persisted gitlabRepoUrl value instead of hardcoding it to
true, ensuring that the validation state reflects the current persisted data on
mount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2580f77-22df-4036-a530-2f0570c3a56c
📒 Files selected for processing (11)
web/.envweb/public/custom-resources/.gitignoreweb/public/custom-resources/README.mdweb/public/custom-resources/my-plugin.jsweb/src/env.tsweb/src/pluginSystem/onyxia_import.tsweb/src/ui/shared/HomeLS3/HomeLS3.tsxweb/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsxweb/src/ui/shared/HomeLS3/HomeLS3GitDialog_backup.tsxweb/src/ui/shared/HomeLS3/HomeLS3InfoCard.tsxweb/src/vite-env.d.ts
💤 Files with no reviewable changes (6)
- web/public/custom-resources/README.md
- web/public/custom-resources/my-plugin.js
- web/.env
- web/public/custom-resources/.gitignore
- web/src/vite-env.d.ts
- web/src/env.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/pluginSystem/onyxia_import.ts
- web/src/ui/shared/HomeLS3/HomeLS3.tsx
| }) { | ||
| const { serviceName, onProceed } = props; | ||
|
|
||
| const { githubPersonalAccessToken } = useCoreState("userConfigs", "userConfigs"); |
Contributor
There was a problem hiding this comment.
Use a GitLab-specific userConfigs key for the PAT.
This dialog stores a GitLab token under githubPersonalAccessToken (Lines 101, 146), which breaks key semantics and can collide with real GitHub credentials in shared user config state.
Proposed fix
- const { githubPersonalAccessToken } = useCoreState("userConfigs", "userConfigs");
+ const { gitlabPersonalAccessToken } = useCoreState("userConfigs", "userConfigs");
...
- const doInjectGitUrl = !!githubPersonalAccessToken && !!gitlabRepoUrl;
+ const doInjectGitUrl = !!gitlabPersonalAccessToken && !!gitlabRepoUrl;
...
- {!githubPersonalAccessToken ? (
+ {!gitlabPersonalAccessToken ? (
...
userConfigs.changeValue({
- key: "githubPersonalAccessToken",
+ key: "gitlabPersonalAccessToken",
value: token
});Also applies to: 145-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog_backup.tsx` at line 101, The
HomeLS3GitDialog_backup component is incorrectly using
`githubPersonalAccessToken` (a GitHub-specific key) to store and retrieve GitLab
credentials, which violates key semantics and risks colliding with actual GitHub
tokens in shared user config state. Replace the `githubPersonalAccessToken` key
with a GitLab-specific key name (such as `gitlabPersonalAccessToken`) in the
useCoreState call and in all other locations where this token is accessed or
stored within the dialog component to ensure proper semantic separation and
prevent credential collisions.
Comment on lines
+127
to
+137
| const [isRepoUrlValid, setIsRepoUrlValid] = useState(true); | ||
|
|
||
| const { gitlabRepoUrl, setGitlabRepoUrl } = useGitlabRepoUrl(); | ||
|
|
||
| const hasGitlabToken = | ||
| normalizeOptionalValue(githubPersonalAccessToken ?? "") !== undefined; | ||
| const normalizedGitlabRepoUrl = normalizeOptionalValue(gitlabRepoUrl ?? ""); | ||
|
|
||
| const doInjectGitUrl = | ||
| hasGitlabToken && normalizedGitlabRepoUrl !== undefined && isRepoUrlValid; | ||
|
|
Contributor
There was a problem hiding this comment.
Initialize repo URL validity from persisted value.
Line 127 sets isRepoUrlValid to true, so an invalid persisted value from Line 253 can make doInjectGitUrl true on first render (Line 136) and allow submitting a malformed URL without any edit.
Proposed fix
- const [isRepoUrlValid, setIsRepoUrlValid] = useState(true);
+ const [isRepoUrlValid, setIsRepoUrlValid] = useState(() => {
+ const initial = normalizeOptionalValue(gitlabRepoUrl ?? "");
+ return initial === undefined || GITLAB_REPO_REGEXP.test(initial);
+ });Also applies to: 253-287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/shared/HomeLS3/HomeLS3GitDialog.tsx` around lines 127 - 137, The
isRepoUrlValid state is initialized to true on the first render, but if there is
a persisted invalid gitlabRepoUrl value, the doInjectGitUrl condition would
still evaluate to true and allow submitting the malformed URL without user
interaction. Initialize isRepoUrlValid based on the actual validity of the
persisted gitlabRepoUrl value instead of hardcoding it to true, ensuring that
the validation state reflects the current persisted data on mount.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/pluginSystem/pluginSystem_legacy.ts`:
- Around line 39-57: The `attachToGlobalIfReady` function can be invoked
concurrently, and multiple calls can pass the initial guard check on line 40
before any async import completes, causing the `onyxiaready` event to be
dispatched multiple times on line 56. To fix this, introduce a guard mechanism
to track whether initialization is already in progress. Add a state variable or
Promise to ensure that once initialization begins, subsequent concurrent calls
either wait for the first initialization to complete or return early without
re-running the initialization logic. This ensures the `onyxiaready` event fires
only once.
In `@web/src/pluginSystem/pluginSystem.ts`:
- Around line 51-69: The lookup in the mount function uses Component reference
equality to find the matching entry, but when the same Component function is
declared multiple times via declareComponent, the find method will always return
the first match instead of the correct one. Create a unique identifier for each
declareComponent call (such as an incrementing ID or object reference), store
this identifier in the entry alongside Component and containerElement, and
return this identifier from declareComponent so it can be captured in the mount
function's closure. Then update the find logic in mount to use this unique
identifier instead of comparing Component references to locate the exact entry
that corresponds to the specific mount call.
- Around line 25-37: The code invokes window.onOnyxiaCtxReady and value_new
without runtime type validation. When the initial check at line 26 verifies that
window.onOnyxiaCtxReady is not undefined but does not verify it is a function
before calling it, a non-function value assigned by external scripts will crash
the plugin-system initialization. Similarly, at line 37 in the setter, value_new
is called without confirming it is a function. Add typeof guards to both
locations: at line 26, change the condition from checking undefined to also
verifying typeof window.onOnyxiaCtxReady === "function" before invoking it; at
line 37 in the setter, add an additional check that typeof value_new ===
"function" before calling it. This ensures only actual callable values are
invoked.
In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx`:
- Line 19: The src attribute on line 19 of HomeLS3Hero.tsx concatenates
PUBLIC_URL directly with the path string without a separating slash, which can
produce a broken URL when PUBLIC_URL does not end with a forward slash. Add a
forward slash between the closing brace of PUBLIC_URL and the word
"custom-resources" to ensure proper URL formation, matching the pattern already
used in HomeLS3.tsx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1af109-eb56-4840-a78b-72a5c7bb1e80
📒 Files selected for processing (8)
web/src/main.lazy.tsxweb/src/pluginSystem/index.tsweb/src/pluginSystem/pluginSystem.tsweb/src/pluginSystem/pluginSystem_legacy.tsweb/src/ui/App/App.tsxweb/src/ui/shared/HomeLS3/HomeLS3.tsxweb/src/ui/shared/HomeLS3/HomeLS3Hero.tsxweb/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ui/shared/HomeLS3/HomeLS3.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/pluginSystem/pluginSystem_legacy.ts`:
- Around line 39-57: The `attachToGlobalIfReady` function can be invoked
concurrently, and multiple calls can pass the initial guard check on line 40
before any async import completes, causing the `onyxiaready` event to be
dispatched multiple times on line 56. To fix this, introduce a guard mechanism
to track whether initialization is already in progress. Add a state variable or
Promise to ensure that once initialization begins, subsequent concurrent calls
either wait for the first initialization to complete or return early without
re-running the initialization logic. This ensures the `onyxiaready` event fires
only once.
In `@web/src/pluginSystem/pluginSystem.ts`:
- Around line 51-69: The lookup in the mount function uses Component reference
equality to find the matching entry, but when the same Component function is
declared multiple times via declareComponent, the find method will always return
the first match instead of the correct one. Create a unique identifier for each
declareComponent call (such as an incrementing ID or object reference), store
this identifier in the entry alongside Component and containerElement, and
return this identifier from declareComponent so it can be captured in the mount
function's closure. Then update the find logic in mount to use this unique
identifier instead of comparing Component references to locate the exact entry
that corresponds to the specific mount call.
- Around line 25-37: The code invokes window.onOnyxiaCtxReady and value_new
without runtime type validation. When the initial check at line 26 verifies that
window.onOnyxiaCtxReady is not undefined but does not verify it is a function
before calling it, a non-function value assigned by external scripts will crash
the plugin-system initialization. Similarly, at line 37 in the setter, value_new
is called without confirming it is a function. Add typeof guards to both
locations: at line 26, change the condition from checking undefined to also
verifying typeof window.onOnyxiaCtxReady === "function" before invoking it; at
line 37 in the setter, add an additional check that typeof value_new ===
"function" before calling it. This ensures only actual callable values are
invoked.
In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx`:
- Line 19: The src attribute on line 19 of HomeLS3Hero.tsx concatenates
PUBLIC_URL directly with the path string without a separating slash, which can
produce a broken URL when PUBLIC_URL does not end with a forward slash. Add a
forward slash between the closing brace of PUBLIC_URL and the word
"custom-resources" to ensure proper URL formation, matching the pattern already
used in HomeLS3.tsx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1af109-eb56-4840-a78b-72a5c7bb1e80
📒 Files selected for processing (8)
web/src/main.lazy.tsxweb/src/pluginSystem/index.tsweb/src/pluginSystem/pluginSystem.tsweb/src/pluginSystem/pluginSystem_legacy.tsweb/src/ui/App/App.tsxweb/src/ui/shared/HomeLS3/HomeLS3.tsxweb/src/ui/shared/HomeLS3/HomeLS3Hero.tsxweb/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ui/shared/HomeLS3/HomeLS3.tsx
🛑 Comments failed to post (4)
web/src/pluginSystem/pluginSystem_legacy.ts (1)
39-57:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate
onyxiareadydispatches during concurrent initialization.Because
attachToGlobalIfReadyis async, multiple invocations can pass the guard on Line 40 before the import resolves, then each dispatch on Line 56. This can initialize legacy listeners multiple times.Suggested fix
+let attachInFlight: Promise<void> | undefined; + const attachToGlobalIfReady = async () => { if (symToStr({ onyxia }) in window) { return; } + + if (attachInFlight !== undefined) { + await attachInFlight; + return; + } + + attachInFlight = (async () => { + if (onyxia.evtGlobalDialog === undefined) { + const { evtGlobalDialog } = await import("ui/App/GlobalDialog"); + onyxia.evtGlobalDialog = evtGlobalDialog; + } + + if (Object.values(onyxia).some(value => value === undefined)) { + return; + } + + if (symToStr({ onyxia }) in window) { + return; + } + + Object.assign(window, { onyxia }); + window.dispatchEvent(new CustomEvent("onyxiaready")); + })(); + + await attachInFlight; + attachInFlight = undefined; - if (onyxia.evtGlobalDialog === undefined) { - const { evtGlobalDialog } = await import("ui/App/GlobalDialog"); - - onyxia.evtGlobalDialog = evtGlobalDialog; - } - - if (Object.values(onyxia).some(value => value === undefined)) { - return; - } - - Object.assign(window, { onyxia }); - - window.dispatchEvent(new CustomEvent("onyxiaready")); };🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 40-40: Prefer
globalThisoverwindow.
[warning] 54-54: Prefer
globalThisoverwindow.
[warning] 50-50: Use
.includes()instead of.some()when checking value existence.
[warning] 56-56: Prefer
globalThisoverwindow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/pluginSystem_legacy.ts` around lines 39 - 57, The `attachToGlobalIfReady` function can be invoked concurrently, and multiple calls can pass the initial guard check on line 40 before any async import completes, causing the `onyxiaready` event to be dispatched multiple times on line 56. To fix this, introduce a guard mechanism to track whether initialization is already in progress. Add a state variable or Promise to ensure that once initialization begins, subsequent concurrent calls either wait for the first initialization to complete or return early without re-running the initialization logic. This ensures the `onyxiaready` event fires only once.web/src/pluginSystem/pluginSystem.ts (2)
25-37:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden callback invocation to avoid startup crashes on invalid global assignments.
At Line 26 and Line 37, the code calls
window.onOnyxiaCtxReadywithout a runtimetypeofguard. If a non-function value is present (from untyped external script), this throws and can break plugin-system init.Suggested fix
- if (window.onOnyxiaCtxReady !== undefined) { + if (typeof window.onOnyxiaCtxReady === "function") { window.onOnyxiaCtxReady(onyxiaCtx); } @@ - set: (value_new: ((onyxiaCtx: OnyxiaCtx) => void) | undefined) => { - if (value_new !== undefined) { + set: (value_new: unknown) => { + if (typeof value_new === "function") { value_new(onyxiaCtx); + value = value_new; + return; } - value = value_new; + if (value_new === undefined) { + value = undefined; + return; + } + value = undefined; }🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 26-26: Prefer
globalThisoverwindow.
[warning] 31-31: Prefer
globalThisoverwindow.
[warning] 29-29: Prefer
globalThisoverwindow.
[warning] 25-25: Prefer
globalThisoverwindow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/pluginSystem.ts` around lines 25 - 37, The code invokes window.onOnyxiaCtxReady and value_new without runtime type validation. When the initial check at line 26 verifies that window.onOnyxiaCtxReady is not undefined but does not verify it is a function before calling it, a non-function value assigned by external scripts will crash the plugin-system initialization. Similarly, at line 37 in the setter, value_new is called without confirming it is a function. Add typeof guards to both locations: at line 26, change the condition from checking undefined to also verifying typeof window.onOnyxiaCtxReady === "function" before invoking it; at line 37 in the setter, add an additional check that typeof value_new === "function" before calling it. This ensures only actual callable values are invoked.
51-69:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
mountcan update the wrong entry when the same component function is declared twice.On Line 61, lookup is by
entry.Component === Component, so twodeclareComponentcalls with the same function object make both mount handles resolve the first match.Suggested fix
const declareComponent: OnyxiaCtx["declareComponent"] = Component => { - evtDeclaredComponents.state.push({ + const declaredComponent = { Component, containerElement: null - }); + }; + evtDeclaredComponents.state = [...evtDeclaredComponents.state, declaredComponent]; return { mount: containerElement => { - const declaredComponents = [...evtDeclaredComponents.state]; - - const declaredComponent = declaredComponents.find( - entry => entry.Component === Component - ); - - assert(declaredComponent !== undefined); - declaredComponent.containerElement = containerElement; - - evtDeclaredComponents.state = declaredComponents; + evtDeclaredComponents.state = [...evtDeclaredComponents.state]; } }; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/pluginSystem.ts` around lines 51 - 69, The lookup in the mount function uses Component reference equality to find the matching entry, but when the same Component function is declared multiple times via declareComponent, the find method will always return the first match instead of the correct one. Create a unique identifier for each declareComponent call (such as an incrementing ID or object reference), store this identifier in the entry alongside Component and containerElement, and return this identifier from declareComponent so it can be captured in the mount function's closure. Then update the find logic in mount to use this unique identifier instead of comparing Component references to locate the exact entry that corresponds to the specific mount call.web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx (1)
19-19:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a stable PUBLIC_URL join for the logo path.
Line 19 concatenates
PUBLIC_URLwithout a separating slash. This can produce a broken URL whenPUBLIC_URLdoes not end with/. Align with the pattern used inweb/src/ui/shared/HomeLS3/HomeLS3.tsx(${PUBLIC_URL}/custom-resources/...).Suggested patch
- src={`${PUBLIC_URL}custom-resources/assets/onyxia-logo-LS3-normal.png`} + src={`${PUBLIC_URL}/custom-resources/assets/onyxia-logo-LS3-normal.png`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.src={`${PUBLIC_URL}/custom-resources/assets/onyxia-logo-LS3-normal.png`}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3Hero.tsx` at line 19, The src attribute on line 19 of HomeLS3Hero.tsx concatenates PUBLIC_URL directly with the path string without a separating slash, which can produce a broken URL when PUBLIC_URL does not end with a forward slash. Add a forward slash between the closing brace of PUBLIC_URL and the word "custom-resources" to ensure proper URL formation, matching the pattern already used in HomeLS3.tsx.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/pluginSystem/pluginSystem.ts (1)
27-37:⚠️ Potential issue | 🟠 MajorDouble initialization will throw
TypeErrordue toconfigurable: false.In development, when
main.lazy.tsxis edited or when its parent module is reloaded during HMR,initPluginSystem()executes again andObject.definePropertyfails because the property is non-configurable.Add an initialization guard to prevent re-execution:
🛡️ Suggested fix: Add initialization guard
+let initialized = false; + export function initPluginSystem() { + if (initialized) { + return; + } + initialized = true; + const onyxiaCtx: OnyxiaCtx = { import: onyxia_import, declareComponent };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/pluginSystem.ts` around lines 27 - 37, The initPluginSystem function attempts to define a non-configurable property on window each time it runs, causing a TypeError during module reloads in development. Add an initialization guard before the Object.defineProperty call that checks if the property already exists on the window object, and skip the defineProperty call if it does. This prevents the error from being thrown when the module is reloaded during HMR while maintaining the intended behavior on first initialization.
♻️ Duplicate comments (2)
web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx (2)
25-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the French typo in the CTA label (Line 25).
Demarershould beDémarrer.Proposed fix
- Demarer un {serviceName} + Démarrer un {serviceName}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx` at line 25, Fix the French spelling error in the HomeLS3ServiceCard component's CTA label. Replace "Demarer" with "Démarrer" in the text string "Demarer un {serviceName}" to correct the missing accent mark and double 'r' in the French verb.
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an
altattribute to the service image (Line 20).The
<img>still has noalt, which is an accessibility issue for screen readers.Proposed fix
- <img className={classes.img} src={coverImageUrl} /> + <img className={classes.img} src={coverImageUrl} alt={`${serviceName} service`} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx` at line 20, The img element with className={classes.img} and src={coverImageUrl} is missing an alt attribute, which is required for accessibility to support screen readers. Add an alt attribute to the img element with a descriptive text that explains what the image represents (for example, the service name or a description of the cover image content).Source: Linters/SAST tools
🧹 Nitpick comments (4)
web/src/ui/shared/HomeLS3/HomeLS3.tsx (2)
30-50: ⚡ Quick winSettle the deferred on cancel instead of leaving the async flow pending forever.
At Line 38, cancel intentionally keeps
dGitlabRepoUrlunresolved, which leavesonServiceClickpending indefinitely. Resolve a cancel sentinel and return before callingroutes.launcher.Proposed patch
- const dGitlabRepoUrl = new Deferred<string | undefined>(); + const dGitlabRepoUrl = new Deferred<string | undefined | null>(); evtGitDialogOpen.post({ serviceName, onUserResponse: params => { switch (params.response) { case "cancel": - // Do nothing, pending forever. + dGitlabRepoUrl.resolve(null); break; case "launch with git repo": dGitlabRepoUrl.resolve(params.gitlabRepoUrl); break; case "launch without git repo": dGitlabRepoUrl.resolve(undefined); break; } } }); const gitlabRepoUrl = await dGitlabRepoUrl.pr; + if (gitlabRepoUrl === null) { + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3.tsx` around lines 30 - 50, In the onServiceClick function, the "cancel" case in the switch statement does nothing, leaving the dGitlabRepoUrl deferred unresolved and causing the function to hang indefinitely when waiting for await dGitlabRepoUrl.pr. Modify the "cancel" case to resolve the deferred with a cancel sentinel value (such as a special error or cancel indicator) instead of doing nothing, which will allow the async flow to continue and the function to return properly.
55-62: ⚡ Quick winDeduplicate per-service metadata to avoid switch drift.
serviceNameis mapped in three different switches (chart name, cover image, title). Centralize this into one typed config object keyed byServiceNameto keep mappings consistent.Proposed refactor sketch
-const serviceNames = ["RStudio", "VSCode"] as const; -type ServiceName = (typeof serviceNames)[number]; +const serviceConfigs = { + RStudio: { + chartName: "rstudio-r-python-julia", + coverImage: "RStudio.jpg", + title: "Pour coder en R" + }, + VSCode: { + chartName: "vscode-r-python-julia", + coverImage: "VSCode.webp", + title: "Pour coder en Python" + } +} as const; +type ServiceName = keyof typeof serviceConfigs; +const serviceNames = Object.keys(serviceConfigs) as ServiceName[];Then replace each switch with
serviceConfigs[serviceName].chartName,.coverImage, and.title.Also applies to: 89-96, 99-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/shared/HomeLS3/HomeLS3.tsx` around lines 55 - 62, The HomeLS3 component contains three separate switch statements that all map serviceName (with cases like "RStudio" and "VSCode") to different properties: chartName, coverImage, and title. This duplication can lead to maintenance issues and inconsistency. Create a single typed configuration object that maps each ServiceName to an object containing all three properties (chartName, coverImage, and title), then replace all three switch statements with direct property lookups from this config object (e.g., serviceConfigs[serviceName].chartName instead of the switch at lines 55-62, serviceConfigs[serviceName].coverImage instead of the switch at lines 89-96, and serviceConfigs[serviceName].title instead of the switch at lines 99-106).web/src/pluginSystem/declareComponent.ts (1)
89-89: ⚡ Quick winPrefer
.datasetoversetAttributefor data attributes.Using the dataset API is more idiomatic and avoids potential XSS vectors with untrusted attribute values.
♻️ Suggested fix
- portalContainerElement.setAttribute("data-onyxia-plugin-portal-container", ""); + portalContainerElement.dataset.onyxiaPluginPortalContainer = "";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/pluginSystem/declareComponent.ts` at line 89, Replace the setAttribute call for the data attribute on portalContainerElement with the dataset API. Instead of portalContainerElement.setAttribute("data-onyxia-plugin-portal-container", ""), use portalContainerElement.dataset.onyxiaPluginPortalContainer = "" to be more idiomatic and avoid potential XSS vectors.Source: Linters/SAST tools
web/src/ui/App/Main.tsx (1)
53-81: ⚡ Quick winWrap plugin components in an ErrorBoundary for isolation.
Plugin components are external code that the host app doesn't control. If a plugin throws during render, the error propagates up and could crash the entire
Maincomponent (or higher), leaving users with a broken UI instead of just a failed plugin.Consider wrapping each portaled component in an ErrorBoundary that renders a fallback (or nothing) on error, ensuring a misbehaving plugin cannot take down the host application.
♻️ Suggested approach
+import { ErrorBoundary } from "react-error-boundary"; // or a custom ErrorBoundary + +function PluginErrorFallback() { + return null; // or a subtle error indicator +} + function CustomComponent() { useRerenderOnStateChange(evtDeclaredComponents); const { classes } = useStyles(); return ( <> {evtDeclaredComponents.state .map(({ Component, containerElement }, i) => containerElement == null ? null : createPortal( - <Suspense - fallback={ - <div className={classes.suspenseFallback}> - <CircularProgress /> - </div> - } - > - <Component /> - </Suspense>, + <ErrorBoundary FallbackComponent={PluginErrorFallback}> + <Suspense + fallback={ + <div className={classes.suspenseFallback}> + <CircularProgress /> + </div> + } + > + <Component /> + </Suspense> + </ErrorBoundary>, containerElement, i ) ) .filter(n => n !== null)} </> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/App/Main.tsx` around lines 53 - 81, The CustomComponent function renders plugin components inside Suspense boundaries, but lacks error handling for when a plugin throws during render. Wrap each portaled Component element with an ErrorBoundary that catches render errors from plugins and displays a fallback UI (such as an error message or nothing at all), preventing a single misbehaving plugin from crashing the entire Main component. Place the ErrorBoundary outside the Suspense boundary so it can catch both Suspense errors and component render errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/pluginSystem/declareComponent.ts`:
- Around line 118-121: The MutationObserver callback registered with
containerState.observer.observe is executing on every DOM mutation app-wide due
to observing document.documentElement with childList and subtree enabled, which
causes syncContainerState to run redundantly during rapid DOM changes. Debounce
the callback function that's passed to the MutationObserver constructor so that
rapid sequential mutations are batched together rather than triggering immediate
syncs. This will eliminate redundant iterations during React render batches and
rapid DOM operations while still capturing when portalContainerElements are
moved externally.
---
Outside diff comments:
In `@web/src/pluginSystem/pluginSystem.ts`:
- Around line 27-37: The initPluginSystem function attempts to define a
non-configurable property on window each time it runs, causing a TypeError
during module reloads in development. Add an initialization guard before the
Object.defineProperty call that checks if the property already exists on the
window object, and skip the defineProperty call if it does. This prevents the
error from being thrown when the module is reloaded during HMR while maintaining
the intended behavior on first initialization.
---
Duplicate comments:
In `@web/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx`:
- Line 25: Fix the French spelling error in the HomeLS3ServiceCard component's
CTA label. Replace "Demarer" with "Démarrer" in the text string "Demarer un
{serviceName}" to correct the missing accent mark and double 'r' in the French
verb.
- Line 20: The img element with className={classes.img} and src={coverImageUrl}
is missing an alt attribute, which is required for accessibility to support
screen readers. Add an alt attribute to the img element with a descriptive text
that explains what the image represents (for example, the service name or a
description of the cover image content).
---
Nitpick comments:
In `@web/src/pluginSystem/declareComponent.ts`:
- Line 89: Replace the setAttribute call for the data attribute on
portalContainerElement with the dataset API. Instead of
portalContainerElement.setAttribute("data-onyxia-plugin-portal-container", ""),
use portalContainerElement.dataset.onyxiaPluginPortalContainer = "" to be more
idiomatic and avoid potential XSS vectors.
In `@web/src/ui/App/Main.tsx`:
- Around line 53-81: The CustomComponent function renders plugin components
inside Suspense boundaries, but lacks error handling for when a plugin throws
during render. Wrap each portaled Component element with an ErrorBoundary that
catches render errors from plugins and displays a fallback UI (such as an error
message or nothing at all), preventing a single misbehaving plugin from crashing
the entire Main component. Place the ErrorBoundary outside the Suspense boundary
so it can catch both Suspense errors and component render errors.
In `@web/src/ui/shared/HomeLS3/HomeLS3.tsx`:
- Around line 30-50: In the onServiceClick function, the "cancel" case in the
switch statement does nothing, leaving the dGitlabRepoUrl deferred unresolved
and causing the function to hang indefinitely when waiting for await
dGitlabRepoUrl.pr. Modify the "cancel" case to resolve the deferred with a
cancel sentinel value (such as a special error or cancel indicator) instead of
doing nothing, which will allow the async flow to continue and the function to
return properly.
- Around line 55-62: The HomeLS3 component contains three separate switch
statements that all map serviceName (with cases like "RStudio" and "VSCode") to
different properties: chartName, coverImage, and title. This duplication can
lead to maintenance issues and inconsistency. Create a single typed
configuration object that maps each ServiceName to an object containing all
three properties (chartName, coverImage, and title), then replace all three
switch statements with direct property lookups from this config object (e.g.,
serviceConfigs[serviceName].chartName instead of the switch at lines 55-62,
serviceConfigs[serviceName].coverImage instead of the switch at lines 89-96, and
serviceConfigs[serviceName].title instead of the switch at lines 99-106).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f23cce3b-2aee-4f54-9634-bc87dd63701c
📒 Files selected for processing (8)
web/.gitignoreweb/src/pluginSystem/declareComponent.tsweb/src/pluginSystem/pluginSystem.tsweb/src/ui/App/App.tsxweb/src/ui/App/Main.tsxweb/src/ui/shared/HomeLS3/HomeLS3.tsxweb/src/ui/shared/HomeLS3/HomeLS3InfoCard.tsxweb/src/ui/shared/HomeLS3/HomeLS3ServiceCard.tsx
💤 Files with no reviewable changes (1)
- web/src/ui/App/App.tsx
✅ Files skipped from review due to trivial changes (1)
- web/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ui/shared/HomeLS3/HomeLS3InfoCard.tsx
Comment on lines
+118
to
+121
| containerState.observer.observe(document.documentElement, { | ||
| childList: true, | ||
| subtree: true | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other MutationObserver usages in the codebase for reference patterns
rg -n "MutationObserver" --type ts --type tsx -C 3Repository: InseeFrLab/onyxia
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check MutationObserver usages - fixing the tsx syntax error
rg -n "MutationObserver" --type ts -C 3Repository: InseeFrLab/onyxia
Length of output: 1834
🏁 Script executed:
#!/bin/bash
# Read the specific file to understand the context
wc -l web/src/pluginSystem/declareComponent.tsRepository: InseeFrLab/onyxia
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Read the declareComponent.ts file around the problematic lines
cat -n web/src/pluginSystem/declareComponent.ts | sed -n '100,140p'Repository: InseeFrLab/onyxia
Length of output: 1576
🏁 Script executed:
#!/bin/bash
# Look at the broader context - portal container management
cat -n web/src/pluginSystem/declareComponent.ts | sed -n '1,50p'Repository: InseeFrLab/onyxia
Length of output: 1796
🏁 Script executed:
#!/bin/bash
# Check how portalContainerElements are populated and used
rg -n "portalContainerElements" web/src/pluginSystem/declareComponent.ts -B 2 -A 2Repository: InseeFrLab/onyxia
Length of output: 1894
🏁 Script executed:
#!/bin/bash
# See the full syncContainerState function
cat -n web/src/pluginSystem/declareComponent.ts | sed -n '128,170p'Repository: InseeFrLab/onyxia
Length of output: 1702
Consider debouncing the MutationObserver callback to avoid redundant syncs.
Observing document.documentElement with { childList: true, subtree: true } causes syncContainerState to run on every DOM mutation app-wide. While the broad scope appears necessary to catch when portalContainerElements are moved externally, the callback can be debounced since rapid sequential mutations don't require rapid re-syncs. This would eliminate redundant iterations of container children during React render batches or rapid DOM operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/pluginSystem/declareComponent.ts` around lines 118 - 121, The
MutationObserver callback registered with containerState.observer.observe is
executing on every DOM mutation app-wide due to observing
document.documentElement with childList and subtree enabled, which causes
syncContainerState to run redundantly during rapid DOM changes. Debounce the
callback function that's passed to the MutationObserver constructor so that
rapid sequential mutations are batched together rather than triggering immediate
syncs. This will eliminate redundant iterations during React render batches and
rapid DOM operations while still capturing when portalContainerElements are
moved externally.
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
1 participant
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.
…as an example usecase
Summary by CodeRabbit