fix npm CVEs and address lint issues in catalog - #1183
Conversation
Signed-off-by: Pranith Rao <pranith.rao@ibm.com>
| @@ -1,6 +1,5 @@ | |||
| import js from '@eslint/js'; | |||
| import tseslint from 'typescript-eslint'; | |||
| import reactPlugin from 'eslint-plugin-react'; | |||
There was a problem hiding this comment.
This PR says it addresses lint issues. Is it addressing them by deleting the use of the react plugin? This removed several checks.
Why is this being removed?
There was a problem hiding this comment.
Explained the reason behind removal in the first point of the PR description
There was a problem hiding this comment.
@ryanmarc Do let me know if this is fine or if I need to follow any other approach...
| const tick = () => updateCountdown(); | ||
| tick(); | ||
| countdownTimerRef.current = setInterval(tick, 1000); |
There was a problem hiding this comment.
tick() is the same call as updateCountdown(), it just hides it from react-hooks/set-state-in-effect. The cascading render the rule is warning about is still there, minus the warning. This PR already uses the honest form in DeploymentDetails.tsx:71; could we do the same here?
| "eslint-plugin-react": "^7.37.5", | ||
| "eslint-plugin-react-hooks": "^7.0.1", | ||
| "eslint-plugin-react-refresh": "^0.4.24", | ||
| "esbuild": "^0.28.1", |
There was a problem hiding this comment.
Is this needed? Nothing in the repo imports esbuild directly, and both main's lockfile and this branch's resolve a single hoisted node_modules/esbuild@0.28.1 (only dependent: vite, range ^0.27.0 || ^0.28.0), so the resolved tree is identical with and without it. If the goal is to floor a transitive version, overrides is the mechanism that actually guarantees it (there's already an overrides block just below for react-table); as a direct devDependency it's a phantom entry that nobody will remember to keep current.
There was a problem hiding this comment.
Removed it. I guess it was added because when I ran npm install multiple times during the eslint/react-router upgrade iterations, esbuild got dropped from node_modules and the build failed with Cannot find package 'esbuild'. Checked fresh run of npm install after removing it now and it succeeded.
| const [loading, setLoading] = useState<boolean>(false); | ||
|
|
||
| const locationState = location.state as LoginLocationState | null; | ||
| const isInactivityLogout = |
There was a problem hiding this comment.
Two things worth noting alongside the set-state-in-effect fix.
(1) sessionStorage.getItem now runs on every render, and the effect on line 44 removes that key, so the component reads mutable external state during render, which is the pattern the react-hooks 7 rules are steering us away from. A lazy initializer (useState(() => isInactivityLogout)) reads it once at mount and keeps render pure.
(2) The banner is now decided only at mount, whereas the old effect could turn it on when location changed. I couldn't find a live path that regresses (we only ever land on /login fresh), but a one-line comment saying "mount-only by design" would stop someone re-introducing the effect later.
There was a problem hiding this comment.
Moved sessionStorage.getItem into a lazy initializer so it runs once at mount and cleans up in the same call. Added the mount-only comment to make the intent explicit.
Signed-off-by: Pranith Rao <pranith.rao@ibm.com>
|
@ryanmarc I have addressed all the review comments PTAL |
|
npm CVEs for all the UI folders are handled in this PR now hence closing this |
- Fixed the npm CVEs across all the UI folders - Synced package.json version ranges across ui/catalog, ui/chatbot, and ui/digitize to match resolved versions in their lock files - Addressed the review comment from [here](#1183 (comment)) https://github.com/user-attachments/assets/1a82f888-25dc-44ae-9bd1-5f74524a2641 --------- Signed-off-by: Pranith Rao <pranith.rao@ibm.com>
2 participants
brace-expansion DoS — caused by eslint@9 pulling in minimatch@3 → fixed by upgrading to eslint@10 which uses safe minimatch@10.
eslint-plugin-reacthad to be removed as it doesn't support eslint@10 yet and CVEs would be present if kept. eslint@10 support addition is tracked here #3979, so once it gets merged we can add it back to our repo.react-router CSRF + 3 other CVEs — no patched v7 release exists, fix only in v8 → migrated from
react-router-dom@7toreact-router@8.3.0, updated all imports accordinglySynced all package.json declared versions to match package-lock.json
Fixed lint errors surfaced by stricter eslint-plugin-react-hooks@7.1.1 rules
Verified all the changes but running
npm i,npm audit,npm run fix && npm run checkandnpm run buildOnce this PR is approved will raise the same fix for
digitizeandchatbotrepos as well