Update plugin system and implement aleternative home for LS3 (Insee) …#1078
Open
garronej wants to merge 2 commits into
Open
Update plugin system and implement aleternative home for LS3 (Insee) …#1078garronej wants to merge 2 commits into
garronej wants to merge 2 commits into
Conversation
…as an example usecase
Contributor
📝 WalkthroughWalkthroughThis PR introduces a plugin mounting system that allows external code to dynamically load and render React components in the main application, implements a shared IDE launcher component with Git configuration support, demonstrates the system with a concrete example plugin, and wires the infrastructure into the app via React portals. ChangesPlugin mounting system and HomeLS3 IDE launcher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
Comment on lines
+6
to
+34
| export function registerPageContainerListener(routeName, listener) { | ||
|
|
||
| /** @type {HTMLElement | undefined} */ | ||
| let element_cache = undefined; | ||
|
|
||
| const update = () => { | ||
| const element = document.getElementById(`page-container-${routeName}`); | ||
| if (element === null) { | ||
| return; | ||
| } | ||
| if (element === element_cache) { | ||
| return; | ||
| } | ||
| element_cache = element; | ||
| listener(element); | ||
| }; | ||
|
|
||
| update(); | ||
|
|
||
| const observer = new MutationObserver(() => { | ||
| update(); | ||
| }); | ||
|
|
||
| observer.observe(document.documentElement, { | ||
| childList: true, | ||
| subtree: true | ||
| }); | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
Memory leak: MutationObserver never disconnected.
The MutationObserver is created and starts observing but is never disconnected. If registerPageContainerListener is called multiple times (e.g., during plugin reloads or if multiple listeners are registered), observers will accumulate in memory and continue running indefinitely, leaking memory and CPU cycles.
🔧 Proposed fix to return cleanup function
/**
* `@param` {string} routeName
* `@param` {(element: HTMLElement) => void} listener
+ * `@returns` {() => void} Cleanup function to disconnect the observer
*/
export function registerPageContainerListener(routeName, listener) {
/** `@type` {HTMLElement | undefined} */
let element_cache = undefined;
const update = () => {
const element = document.getElementById(`page-container-${routeName}`);
if (element === null) {
return;
}
if (element === element_cache) {
return;
}
element_cache = element;
listener(element);
};
update();
const observer = new MutationObserver(() => {
update();
});
observer.observe(document.documentElement, {
childList: true,
subtree: true
});
+
+ // Return cleanup function
+ return () => {
+ observer.disconnect();
+ };
}Callers should store and invoke the returned cleanup function when appropriate (e.g., on plugin unload or route change).
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 9-9: It's not necessary to initialize 'element_cache' to undefined.
🤖 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/registerPageContainerListener.js`
around lines 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.
| 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
Comment on lines
+62
to
+70
| helmValuesPatch: | ||
| gitlabRepoUrl === undefined | ||
| ? undefined | ||
| : [ | ||
| { | ||
| path: ["git", "repository"], | ||
| value: "https://github.com/InseeFrLab/onyxia" | ||
| } | ||
| ], |
Contributor
There was a problem hiding this comment.
Critical bug: User-provided GitLab repository URL is ignored.
The helmValuesPatch sets git.repository to a hardcoded GitHub URL ("https://github.com/InseeFrLab/onyxia") instead of using the gitlabRepoUrl value collected from the user via the dialog. This completely defeats the purpose of the GitLab integration flow where users provide their own repository URL.
🐛 Proposed fix
helmValuesPatch:
gitlabRepoUrl === undefined
? undefined
: [
{
path: ["git", "repository"],
- value: "https://github.com/InseeFrLab/onyxia"
+ value: gitlabRepoUrl
}
],🤖 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 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.
| }) { | ||
| 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.
Comment on lines
+156
to
+158
| Un Token GitLab doit être de la forme | ||
| gplat-xxx | ||
| </> |
Contributor
There was a problem hiding this comment.
Typo in validation error message.
The error message contains "gplat-xxx" but the correct GitLab token prefix is "glpat-xxx" (with 'l' before 'p').
📝 Proposed fix
Un Token GitLab doit être de la forme
- gplat-xxx
+ glpat-xxx🤖 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 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.
|
|
||
| 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.
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
New Features
Documentation
Refactor