[ES-2894] Updating react app to 19 version#1649
Open
zesu22 wants to merge 3 commits intomosip:developfrom
Open
[ES-2894] Updating react app to 19 version#1649zesu22 wants to merge 3 commits intomosip:developfrom
zesu22 wants to merge 3 commits intomosip:developfrom
Conversation
WalkthroughAdds a CRACO webpack configuration to provide browser polyfills and shims, upgrades React and many dependencies, migrates routing imports from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
oidc-ui/src/pages/SomethingWrong.js (1)
7-7:⚠️ Potential issue | 🟡 MinorPotential runtime error when
location.stateis null.If a user navigates directly to this page (e.g., via URL) rather than through programmatic navigation with state,
useLocation().statewill benull, causing aTypeErrorwhen accessing.code.🛡️ Proposed defensive access
- const statusCode = useLocation().state.code; + const location = useLocation(); + const statusCode = location.state?.code ?? 'unknown';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/pages/SomethingWrong.js` at line 7, The line extracting statusCode can throw if useLocation().state is null; change the extraction in SomethingWrong.js to defensively read location state (useLocation) with optional chaining and a sensible default (e.g., null or a fallback status like 500) and then use that safe value wherever statusCode is used; update any rendering logic that assumes a number to handle the fallback case. Ensure you reference useLocation() and the statusCode variable so the fix is applied where that const is declared and consumed.oidc-ui/src/test/components/OtpVerify.test.js (2)
183-191:⚠️ Potential issue | 🟠 MajorPer-test
jest.mockhere cannot override the cached module.
OtpVerifyis imported at the top level (line 48), so by the time the test runs and callsjest.mock(line 183), the module is already cached in Jest's module registry. The subsequentawait import(line 191) retrieves the cached version with the originalreact-i18nextmock from line 6, not the new mock defined in the test. This defeats the isolation and gives misleading coverage for the translated-label branch.Fix: Call
jest.resetModules()beforejest.doMock()to clear the cache and load a fresh module with the test-specific mock:Safer isolation approach
it('returns translated OTP label when available', async () => { + jest.resetModules(); - jest.mock('react-i18next', () => ({ + jest.doMock('react-i18next', () => ({ useTranslation: () => ({ t: (key) => (key === 'otp_label_text' ? 'Enter OTP' : key), i18n: { language: 'en', on: jest.fn() }, }), })); const { default: OtpVerifyLabelTest } = await import('../../components/OtpVerify');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/components/OtpVerify.test.js` around lines 183 - 191, The test-level mock for react-i18next doesn't take effect because OtpVerify was already imported and cached; before creating the per-test mock, call jest.resetModules() then use jest.doMock('react-i18next', ...) (or jest.mock after reset) so the subsequent dynamic import of '../../components/OtpVerify' returns a fresh module wired to the test's translation mock; update the test around the use of useTranslation/otp_label_text mock, replace the current jest.mock + await import sequence with resetModules() followed by doMock/mock and then await import of OtpVerify to ensure the translated-label branch is exercised.
132-137:⚠️ Potential issue | 🟠 MajorTests query obsolete
.pincode-input-textselectors that don't exist in the currentinput-otpcomponent.The component now uses the
input-otplibrary which renders an invisible backing<input>with adata-input-otpattribute plus custom "slot" UI elements. The test selectors target.pincode-input-text(from the oldreact-pin-inputlibrary) which are never rendered, causing the OTP entry flow to not be tested at all.Replace all three instances with:
Suggested fix
- const inputs = document.querySelectorAll('.pincode-input-text'); - expect(inputs.length).toBe(6); - - for (let i = 0; i < inputs.length; i++) { - await userEvent.type(inputs[i], `${i + 1}`); - } + const otpInput = document.querySelector('input[data-input-otp]'); + expect(otpInput).toBeInTheDocument(); + await userEvent.type(otpInput, '123456');Affects lines 132-137, 206-209, 235-238.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/components/OtpVerify.test.js` around lines 132 - 137, Tests currently query the old '.pincode-input-text' elements which no longer exist; update each instance to target the input-otp backing input by selecting '[data-input-otp]' (e.g. const otpInput = document.querySelector('[data-input-otp]')) and assert it exists, then simulate typing the full OTP into that single backing input with userEvent.type(otpInput, '123456') (replace the loop and inputs.length expectation). Apply this replacement for all three occurrences in OtpVerify.test.js that use '.pincode-input-text'.
🧹 Nitpick comments (1)
oidc-ui/src/test/components/L1Biometrics.test.js (1)
107-120: Mock signature may not work correctly with ref forwarding.The mock uses a function signature
({ onChange }, ref)suggesting it expects to receive a forwarded ref, but it's not wrapped inReact.forwardRef(). If the actualreact-google-recaptchacomponent uses ref forwarding (which it does for methods likereset()), this mock won't properly receive the ref.🔧 Suggested fix to properly forward ref
jest.mock('react-google-recaptcha', () => { + const React = require('react'); - const MockRecaptcha = ({ onChange }, ref) => ( + const MockRecaptcha = React.forwardRef(({ onChange }, ref) => ( <div data-testid="recaptcha" ref={ref}> <button data-testid="captcha-button" onClick={() => onChange('captcha-token')} > Complete CAPTCHA </button> </div> - ); + )); MockRecaptcha.displayName = 'MockRecaptcha'; return MockRecaptcha; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/components/L1Biometrics.test.js` around lines 107 - 120, The mock for react-google-recaptcha uses a ref parameter but doesn't forward it, so tests expecting ref methods (e.g., reset) will fail; update the mock in L1Biometrics.test.js to wrap MockRecaptcha with React.forwardRef so it accepts (props, ref) properly, keep the props usage (onChange) and attach the forwarded ref to the root element, and preserve MockRecaptcha.displayName so it remains identifiable in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@oidc-ui/src/components/LoginIDOptions.js`:
- Around line 113-115: The effect that calls
props.currentLoginID(selectedOption) is missing props.currentLoginID in its
dependency array; update the useEffect in LoginIDOptions.js (the effect that
references selectedOption) so the dependency array includes both selectedOption
and props.currentLoginID (i.e., [selectedOption, props.currentLoginID]) to
ensure the latest callback reference is invoked when the parent re-renders.
In `@oidc-ui/src/components/OtpVerify.js`:
- Around line 414-433: The component now uses the controlled OTPInput via
value={otpValue} and onChange={setOtpValue}, but leftover imperative usage of
the pin ref (pin.clear()) is stale and causes runtime errors because pin is
never assigned; remove the pin ref declaration and the pin.clear() call (keep
the existing setOtpValue('') to clear the input) and ensure all references to
pin are deleted so OTPInput, otpValue, and setOtpValue are the single source of
truth for clearing and managing the OTP.
---
Outside diff comments:
In `@oidc-ui/src/pages/SomethingWrong.js`:
- Line 7: The line extracting statusCode can throw if useLocation().state is
null; change the extraction in SomethingWrong.js to defensively read location
state (useLocation) with optional chaining and a sensible default (e.g., null or
a fallback status like 500) and then use that safe value wherever statusCode is
used; update any rendering logic that assumes a number to handle the fallback
case. Ensure you reference useLocation() and the statusCode variable so the fix
is applied where that const is declared and consumed.
In `@oidc-ui/src/test/components/OtpVerify.test.js`:
- Around line 183-191: The test-level mock for react-i18next doesn't take effect
because OtpVerify was already imported and cached; before creating the per-test
mock, call jest.resetModules() then use jest.doMock('react-i18next', ...) (or
jest.mock after reset) so the subsequent dynamic import of
'../../components/OtpVerify' returns a fresh module wired to the test's
translation mock; update the test around the use of
useTranslation/otp_label_text mock, replace the current jest.mock + await import
sequence with resetModules() followed by doMock/mock and then await import of
OtpVerify to ensure the translated-label branch is exercised.
- Around line 132-137: Tests currently query the old '.pincode-input-text'
elements which no longer exist; update each instance to target the input-otp
backing input by selecting '[data-input-otp]' (e.g. const otpInput =
document.querySelector('[data-input-otp]')) and assert it exists, then simulate
typing the full OTP into that single backing input with userEvent.type(otpInput,
'123456') (replace the loop and inputs.length expectation). Apply this
replacement for all three occurrences in OtpVerify.test.js that use
'.pincode-input-text'.
---
Nitpick comments:
In `@oidc-ui/src/test/components/L1Biometrics.test.js`:
- Around line 107-120: The mock for react-google-recaptcha uses a ref parameter
but doesn't forward it, so tests expecting ref methods (e.g., reset) will fail;
update the mock in L1Biometrics.test.js to wrap MockRecaptcha with
React.forwardRef so it accepts (props, ref) properly, keep the props usage
(onChange) and attach the forwarded ref to the root element, and preserve
MockRecaptcha.displayName so it remains identifiable in tests.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
oidc-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
oidc-ui/craco.config.jsoidc-ui/package.jsonoidc-ui/src/App.jsoidc-ui/src/app/AppRouter.jsoidc-ui/src/common/ErrorIndicator.jsoidc-ui/src/common/LoadingIndicator.jsoidc-ui/src/components/Authorize.jsoidc-ui/src/components/Form.jsoidc-ui/src/components/IdToken.jsoidc-ui/src/components/L1Biometrics.jsoidc-ui/src/components/LoginIDOptions.jsoidc-ui/src/components/OtpVerify.jsoidc-ui/src/components/Password.jsoidc-ui/src/components/Pin.jsoidc-ui/src/index.jsoidc-ui/src/pages/Consent.jsoidc-ui/src/pages/Login.jsoidc-ui/src/pages/NetworkError.jsoidc-ui/src/pages/SomethingWrong.jsoidc-ui/src/setupTests.jsoidc-ui/src/test/app/AppRouter.test.jsoidc-ui/src/test/commons/ErrorIndicator.test.jsoidc-ui/src/test/components/Authorize.test.jsoidc-ui/src/test/components/Form.test.jsoidc-ui/src/test/components/IdToken.test.jsoidc-ui/src/test/components/InputWithImage.test.jsoidc-ui/src/test/components/L1Biometrics.test.jsoidc-ui/src/test/components/OtpVerify.test.jsoidc-ui/src/test/components/Password.test.jsoidc-ui/src/test/components/Pin.test.jsoidc-ui/src/test/pages/Consent.test.jsoidc-ui/src/test/pages/Login.test.jsoidc-ui/src/test/pages/NetworkError.test.jsoidc-ui/src/test/pages/PageNotFound.test.jsoidc-ui/src/test/pages/SomethingWrong.test.jsoidc-ui/tailwind.config.js
💤 Files with no reviewable changes (1)
- oidc-ui/src/test/components/InputWithImage.test.js
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oidc-ui/src/test/components/L1Biometrics.test.js (1)
108-117:⚠️ Potential issue | 🟠 MajorFix React 19 ref handling in reCAPTCHA test mock.
Line 108 uses
({ onChange }, ref), but function components don't receiverefas a second parameter withoutReact.forwardRef(). The actual component calls_reCaptchaRef.current.reset()(line 289 in L1Biometrics.js), which will fail silently against this mock sincerefwill be undefined. Update the mock to accept ref as a prop and expose thereset()method viauseImperativeHandle():Proposed fix
jest.mock('react-google-recaptcha', () => { - const MockRecaptcha = ({ onChange }, ref) => ( - <div data-testid="recaptcha" ref={ref}> + const React = require('react'); + const MockRecaptcha = ({ onChange, ref: captchaRef }) => { + React.useImperativeHandle(captchaRef, () => ({ + reset: jest.fn(), + execute: jest.fn(), + executeAsync: jest.fn().mockResolvedValue('captcha-token'), + })); + return ( + <div data-testid="recaptcha"> <button data-testid="captcha-button" onClick={() => onChange('captcha-token')} > Complete CAPTCHA </button> - </div> - ); + </div> + ); + }; MockRecaptcha.displayName = 'MockRecaptcha'; return MockRecaptcha; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/components/L1Biometrics.test.js` around lines 108 - 117, The MockRecaptcha test component currently declares its signature as ({ onChange }, ref) which doesn't receive ref in React 18/19; wrap/convert MockRecaptcha to use React.forwardRef so it receives forwarded ref and inside the component use React.useImperativeHandle to expose a reset() method on the forwarded ref (so calls to _reCaptchaRef.current.reset() in L1Biometrics.js won't fail); ensure the exposed reset method performs the expected mock behavior (e.g., clears or resets the token / calls onChange(null) or a noop) and keep the existing data-testid attributes unchanged.
♻️ Duplicate comments (1)
oidc-ui/src/components/LoginIDOptions.js (1)
113-115:⚠️ Potential issue | 🟡 MinorMissing
props.currentLoginIDin dependency array; effect may also fire withundefined.Two issues here:
The dependency array should include
props.currentLoginIDto ensure the effect uses the latest callback reference when the parent re-renders.On initial mount,
selectedOptionisundefined, and since the icon preloading effect (lines 69-87) is async, this effect will fire first withundefined. Consider adding a guard to avoid calling the parent callback with an invalid value.Suggested fix
useEffect(() => { + if (selectedOption) { props.currentLoginID(selectedOption); - }, [selectedOption]); + } + }, [selectedOption, props.currentLoginID]);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/components/LoginIDOptions.js` around lines 113 - 115, The effect using useEffect should include props.currentLoginID in its dependency array and guard against calling the parent with an invalid value: update the effect that currently reads useEffect(() => { props.currentLoginID(selectedOption); }, [selectedOption]); to add props.currentLoginID to dependencies and add a check that selectedOption is not undefined/null (or otherwise valid) before invoking props.currentLoginID; reference the useEffect, props.currentLoginID, and selectedOption symbols when making the change so the effect won't fire with an undefined value and will use the latest callback reference.
🧹 Nitpick comments (3)
oidc-ui/package.json (2)
16-16: Consider removingcra-templatefrom dependencies.
cra-templateis typically used only for scaffolding new Create React App projects and is not needed as a runtime or build dependency. Unless there's a specific reason for its inclusion, it can be safely removed.♻️ Proposed fix
"dependencies": { - "cra-template": "1.3.0", ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/package.json` at line 16, Remove the "cra-template" dependency from package.json's dependencies (the "cra-template": "1.3.0" entry) since it's a scaffolding-only template and not needed at runtime; after removing the entry, run your package manager (npm install or yarn install) to update node_modules and the lockfile, and verify there are no build or CI scripts that rely on "cra-template" before merging.
6-6: Move@craco/cracoto devDependencies.
@craco/cracois a build-time tool and should be listed underdevDependenciesrather thandependencies. This keeps the production bundle smaller and correctly categorizes the package's purpose.♻️ Proposed fix
Move from
dependenciestodevDependencies:"dependencies": { - "@craco/craco": "^7.1.0", "@emotion/react": "^11.14.0", ... }, "devDependencies": { + "@craco/craco": "^7.1.0", "@testing-library/jest-dom": "^6.9.1", ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/package.json` at line 6, The package.json currently lists "@craco/craco" under dependencies; move "@craco/craco" into devDependencies instead (remove it from the dependencies object and add it with the same version string to devDependencies) so it is only installed for build/dev environments; update the package.json entry for "@craco/craco" accordingly.oidc-ui/src/test/pages/SomethingWrong.test.js (1)
5-12: LGTM!The mock correctly targets
react-routerand uses therequireActualpattern to preserve real exports while overridinguseLocation.Minor consistency note: This file uses
originalModuleas the variable name, while other test files in this PR useactual. Consider aligning the naming for consistency across the test suite, though this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/pages/SomethingWrong.test.js` around lines 5 - 12, Rename the local variable used when calling jest.requireActual in the react-router mock to match the project's convention; replace originalModule with actual in the jest.mock block that overrides useLocation so the mock reads: const actual = jest.requireActual('react-router'); and then spread ...actual before overriding useLocation to keep naming consistent with other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@oidc-ui/package.json`:
- Around line 33-34: Remove the redundant dependency "react-router-dom" from
package.json since the codebase imports everything directly from "react-router"
(e.g., BrowserRouter, Route, Routes, MemoryRouter and hooks); update
package.json by deleting the "react-router-dom" entry and run package manager
install (npm/yarn/pnpm) to update lockfiles, then run the test/build to ensure
nothing breaks and commit the updated package.json and lockfile changes.
In `@oidc-ui/src/test/components/OtpVerify.test.js`:
- Around line 190-191: The dynamic import of OtpVerify (assigned to
OtpVerifyLabelTest) can return a cached module because OtpVerify is also
imported at the top of the file; instead wrap the per-test import in a module
isolation block and mock before import so the mock is applied reliably: use
jest.isolateModules to create a fresh module registry and call
jest.doMock('../../components/OtpVerify', ...) inside that isolation block, then
import the module (assign to OtpVerifyLabelTest) within the same isolate
callback; ensure you remove or avoid the conflicting top-level import of
OtpVerify so the isolated import loads the mocked version.
---
Outside diff comments:
In `@oidc-ui/src/test/components/L1Biometrics.test.js`:
- Around line 108-117: The MockRecaptcha test component currently declares its
signature as ({ onChange }, ref) which doesn't receive ref in React 18/19;
wrap/convert MockRecaptcha to use React.forwardRef so it receives forwarded ref
and inside the component use React.useImperativeHandle to expose a reset()
method on the forwarded ref (so calls to _reCaptchaRef.current.reset() in
L1Biometrics.js won't fail); ensure the exposed reset method performs the
expected mock behavior (e.g., clears or resets the token / calls onChange(null)
or a noop) and keep the existing data-testid attributes unchanged.
---
Duplicate comments:
In `@oidc-ui/src/components/LoginIDOptions.js`:
- Around line 113-115: The effect using useEffect should include
props.currentLoginID in its dependency array and guard against calling the
parent with an invalid value: update the effect that currently reads
useEffect(() => { props.currentLoginID(selectedOption); }, [selectedOption]); to
add props.currentLoginID to dependencies and add a check that selectedOption is
not undefined/null (or otherwise valid) before invoking props.currentLoginID;
reference the useEffect, props.currentLoginID, and selectedOption symbols when
making the change so the effect won't fire with an undefined value and will use
the latest callback reference.
---
Nitpick comments:
In `@oidc-ui/package.json`:
- Line 16: Remove the "cra-template" dependency from package.json's dependencies
(the "cra-template": "1.3.0" entry) since it's a scaffolding-only template and
not needed at runtime; after removing the entry, run your package manager (npm
install or yarn install) to update node_modules and the lockfile, and verify
there are no build or CI scripts that rely on "cra-template" before merging.
- Line 6: The package.json currently lists "@craco/craco" under dependencies;
move "@craco/craco" into devDependencies instead (remove it from the
dependencies object and add it with the same version string to devDependencies)
so it is only installed for build/dev environments; update the package.json
entry for "@craco/craco" accordingly.
In `@oidc-ui/src/test/pages/SomethingWrong.test.js`:
- Around line 5-12: Rename the local variable used when calling
jest.requireActual in the react-router mock to match the project's convention;
replace originalModule with actual in the jest.mock block that overrides
useLocation so the mock reads: const actual =
jest.requireActual('react-router'); and then spread ...actual before overriding
useLocation to keep naming consistent with other tests.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
oidc-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
oidc-ui/craco.config.jsoidc-ui/package.jsonoidc-ui/src/App.jsoidc-ui/src/app/AppRouter.jsoidc-ui/src/common/ErrorIndicator.jsoidc-ui/src/common/LoadingIndicator.jsoidc-ui/src/components/Authorize.jsoidc-ui/src/components/Form.jsoidc-ui/src/components/IdToken.jsoidc-ui/src/components/L1Biometrics.jsoidc-ui/src/components/LoginIDOptions.jsoidc-ui/src/components/OtpVerify.jsoidc-ui/src/components/Password.jsoidc-ui/src/components/Pin.jsoidc-ui/src/index.jsoidc-ui/src/pages/Consent.jsoidc-ui/src/pages/Login.jsoidc-ui/src/pages/NetworkError.jsoidc-ui/src/pages/SomethingWrong.jsoidc-ui/src/setupTests.jsoidc-ui/src/test/app/AppRouter.test.jsoidc-ui/src/test/commons/ErrorIndicator.test.jsoidc-ui/src/test/components/Authorize.test.jsoidc-ui/src/test/components/Form.test.jsoidc-ui/src/test/components/IdToken.test.jsoidc-ui/src/test/components/InputWithImage.test.jsoidc-ui/src/test/components/L1Biometrics.test.jsoidc-ui/src/test/components/OtpVerify.test.jsoidc-ui/src/test/components/Password.test.jsoidc-ui/src/test/components/Pin.test.jsoidc-ui/src/test/pages/Consent.test.jsoidc-ui/src/test/pages/Login.test.jsoidc-ui/src/test/pages/NetworkError.test.jsoidc-ui/src/test/pages/PageNotFound.test.jsoidc-ui/src/test/pages/SomethingWrong.test.jsoidc-ui/tailwind.config.js
💤 Files with no reviewable changes (1)
- oidc-ui/src/test/components/InputWithImage.test.js
🚧 Files skipped from review as they are similar to previous changes (18)
- oidc-ui/src/test/app/AppRouter.test.js
- oidc-ui/src/test/components/Password.test.js
- oidc-ui/src/test/components/IdToken.test.js
- oidc-ui/src/pages/Consent.js
- oidc-ui/src/App.js
- oidc-ui/src/test/commons/ErrorIndicator.test.js
- oidc-ui/src/test/components/Authorize.test.js
- oidc-ui/src/app/AppRouter.js
- oidc-ui/src/index.js
- oidc-ui/src/pages/Login.js
- oidc-ui/src/test/components/Pin.test.js
- oidc-ui/src/components/Password.js
- oidc-ui/src/components/OtpVerify.js
- oidc-ui/src/test/pages/Consent.test.js
- oidc-ui/src/pages/SomethingWrong.js
- oidc-ui/src/components/IdToken.js
- oidc-ui/src/components/L1Biometrics.js
- oidc-ui/src/pages/NetworkError.js
Comment on lines
+33
to
+34
| "react-router": "^7.13.1", | ||
| "react-router-dom": "^7.13.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if react-router-dom is still imported anywhere in the source code
# Search for react-router-dom imports
echo "=== Imports from react-router-dom ==="
rg -n "from ['\"]react-router-dom['\"]" --type js --type jsx
echo ""
echo "=== Requires of react-router-dom ==="
rg -n "require\(['\"]react-router-dom['\"]" --type js
echo ""
echo "=== Any reference to react-router-dom ==="
rg "react-router-dom" --type js --type jsxRepository: mosip/esignet
Length of output: 248
🏁 Script executed:
#!/bin/bash
# More comprehensive search without file type restrictions
echo "=== Searching for react-router-dom (any file type) ==="
rg "react-router-dom" --type-list | head -20
echo ""
echo "=== Searching all files for react-router-dom ==="
rg "react-router-dom" | head -30
echo ""
echo "=== Content of package.json (dependencies section) ==="
cat oidc-ui/package.json | grep -A 20 '"dependencies"'Repository: mosip/esignet
Length of output: 1810
🏁 Script executed:
#!/bin/bash
# Check what react-router imports are used (both with and without -dom)
echo "=== Imports from react-router (non-dom) ==="
rg "from ['\"]react-router['\"]"
echo ""
echo "=== All react-router related imports ==="
rg "from ['\"]react-router"
echo ""
echo "=== Showing full package.json for context ==="
head -50 oidc-ui/package.jsonRepository: mosip/esignet
Length of output: 6174
Remove react-router-dom from dependencies.
All imports have been migrated to react-router. The codebase imports 25+ components and hooks directly from react-router (including BrowserRouter, Route, Routes, MemoryRouter, and various hooks), while there are zero imports of react-router-dom anywhere in the code. Since react-router-dom re-exports everything from react-router in v7, this dependency is redundant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@oidc-ui/package.json` around lines 33 - 34, Remove the redundant dependency
"react-router-dom" from package.json since the codebase imports everything
directly from "react-router" (e.g., BrowserRouter, Route, Routes, MemoryRouter
and hooks); update package.json by deleting the "react-router-dom" entry and run
package manager install (npm/yarn/pnpm) to update lockfiles, then run the
test/build to ensure nothing breaks and commit the updated package.json and
lockfile changes.
Comment on lines
+190
to
+191
| const { default: OtpVerifyLabelTest } = | ||
| await import('../../components/OtpVerify'); |
There was a problem hiding this comment.
This dynamic import likely reuses cached OtpVerify, so the test-specific mock is not reliably applied.
OtpVerify is already imported earlier in the file, so this import path can return the cached module and bypass intended per-test mocking behavior.
✅ Safer pattern (isolate module + doMock)
- const { default: OtpVerifyLabelTest } =
- await import('../../components/OtpVerify');
+ jest.resetModules();
+ jest.doMock('react-i18next', () => ({
+ useTranslation: () => ({
+ t: (key) => (key === 'otp_label_text' ? 'Enter OTP' : key),
+ i18n: { language: 'en', on: jest.fn() },
+ }),
+ }));
+ const { default: OtpVerifyLabelTest } = await import('../../components/OtpVerify');#!/bin/bash
# Verify that the module is both top-level imported and dynamically imported in the same test file.
rg -n -C2 "import OtpVerify from '../../components/OtpVerify'|await import\('../../components/OtpVerify'\)|jest\.mock\('react-i18next'" oidc-ui/src/test/components/OtpVerify.test.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@oidc-ui/src/test/components/OtpVerify.test.js` around lines 190 - 191, The
dynamic import of OtpVerify (assigned to OtpVerifyLabelTest) can return a cached
module because OtpVerify is also imported at the top of the file; instead wrap
the per-test import in a module isolation block and mock before import so the
mock is applied reliably: use jest.isolateModules to create a fresh module
registry and call jest.doMock('../../components/OtpVerify', ...) inside that
isolation block, then import the module (assign to OtpVerifyLabelTest) within
the same isolate callback; ensure you remove or avoid the conflicting top-level
import of OtpVerify so the isolated import loads the mocked version.
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
oidc-ui/src/test/components/OtpVerify.test.js (1)
183-191:⚠️ Potential issue | 🟠 MajorIsolate module loading for test-local i18n mock.
OtpVerifyis already imported at Line 48, so the dynamic import at Line 190 can reuse cached module state and ignore the mock declared in this test block.Safer isolation approach
- jest.mock('react-i18next', () => ({ - useTranslation: () => ({ - t: (key) => (key === 'otp_label_text' ? 'Enter OTP' : key), - i18n: { language: 'en', on: jest.fn() }, - }), - })); - - const { default: OtpVerifyLabelTest } = - await import('../../components/OtpVerify'); + let OtpVerifyLabelTest; + jest.isolateModules(() => { + jest.doMock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key) => (key === 'otp_label_text' ? 'Enter OTP' : key), + i18n: { language: 'en', on: jest.fn() }, + }), + })); + OtpVerifyLabelTest = require('../../components/OtpVerify').default; + });#!/bin/bash # Verify cached-import risk pattern in the same test file rg -n -C2 "import OtpVerify from '../../components/OtpVerify'|await import\\('../../components/OtpVerify'\\)|jest\\.mock\\('react-i18next'" oidc-ui/src/test/components/OtpVerify.test.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/test/components/OtpVerify.test.js` around lines 183 - 191, The test defines a test-local i18n mock but still dynamically imports OtpVerify after the module was already imported earlier (OtpVerify default import at top), so the mock is ignored; fix by isolating module loading: before dynamically importing or requiring '../../components/OtpVerify' inside this test block, call jest.resetModules() (or wrap the import in jest.isolateModules) and then declare the jest.mock('react-i18next', ...) so the module cache is cleared and the subsequent import of OtpVerify (reference the dynamic import variable OtpVerifyLabelTest or the file's default export) picks up the test-local mock.oidc-ui/src/components/OtpVerify.js (1)
102-103:⚠️ Potential issue | 🔴 CriticalRemove stale
pin.clear()from resend flow.At Line 132,
pin.clear()is still called, but the component is now controlled viaotpValue/setOtpValue. This can throw and force the resend path into the catch block.Proposed fix
- let pin = useRef(); @@ const sendOTP = async () => { try { setErrorBanner(null); - pin.clear(); setOtpValue('');#!/bin/bash # Verify stale ref-clear pattern still exists rg -n "let pin = useRef|pin\\.clear\\(" oidc-ui/src/components/OtpVerify.jsAlso applies to: 129-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oidc-ui/src/components/OtpVerify.js` around lines 102 - 103, The resend flow still calls the stale ref method pin.clear(), which throws because the component is now controlled by otpValue/setOtpValue; remove any pin.clear() calls (reference: the pin ref and the resend handler around where otpValue and setOtpValue are used) and instead reset the controlled value by calling setOtpValue("") (or appropriate initial value) in the resend path; also remove/clean up the unused let pin = useRef() declaration if nothing else uses that ref.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@oidc-ui/src/components/OtpVerify.js`:
- Around line 102-103: The resend flow still calls the stale ref method
pin.clear(), which throws because the component is now controlled by
otpValue/setOtpValue; remove any pin.clear() calls (reference: the pin ref and
the resend handler around where otpValue and setOtpValue are used) and instead
reset the controlled value by calling setOtpValue("") (or appropriate initial
value) in the resend path; also remove/clean up the unused let pin = useRef()
declaration if nothing else uses that ref.
In `@oidc-ui/src/test/components/OtpVerify.test.js`:
- Around line 183-191: The test defines a test-local i18n mock but still
dynamically imports OtpVerify after the module was already imported earlier
(OtpVerify default import at top), so the mock is ignored; fix by isolating
module loading: before dynamically importing or requiring
'../../components/OtpVerify' inside this test block, call jest.resetModules()
(or wrap the import in jest.isolateModules) and then declare the
jest.mock('react-i18next', ...) so the module cache is cleared and the
subsequent import of OtpVerify (reference the dynamic import variable
OtpVerifyLabelTest or the file's default export) picks up the test-local mock.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
oidc-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
oidc-ui/craco.config.jsoidc-ui/package.jsonoidc-ui/src/App.jsoidc-ui/src/app/AppRouter.jsoidc-ui/src/common/ErrorIndicator.jsoidc-ui/src/common/LoadingIndicator.jsoidc-ui/src/components/Authorize.jsoidc-ui/src/components/Form.jsoidc-ui/src/components/IdToken.jsoidc-ui/src/components/L1Biometrics.jsoidc-ui/src/components/LoginIDOptions.jsoidc-ui/src/components/OtpVerify.jsoidc-ui/src/components/Password.jsoidc-ui/src/components/Pin.jsoidc-ui/src/index.jsoidc-ui/src/pages/Consent.jsoidc-ui/src/pages/Login.jsoidc-ui/src/pages/NetworkError.jsoidc-ui/src/pages/SomethingWrong.jsoidc-ui/src/setupTests.jsoidc-ui/src/test/app/AppRouter.test.jsoidc-ui/src/test/commons/ErrorIndicator.test.jsoidc-ui/src/test/components/Authorize.test.jsoidc-ui/src/test/components/Form.test.jsoidc-ui/src/test/components/IdToken.test.jsoidc-ui/src/test/components/InputWithImage.test.jsoidc-ui/src/test/components/L1Biometrics.test.jsoidc-ui/src/test/components/OtpVerify.test.jsoidc-ui/src/test/components/Password.test.jsoidc-ui/src/test/components/Pin.test.jsoidc-ui/src/test/pages/Consent.test.jsoidc-ui/src/test/pages/Login.test.jsoidc-ui/src/test/pages/NetworkError.test.jsoidc-ui/src/test/pages/PageNotFound.test.jsoidc-ui/src/test/pages/SomethingWrong.test.jsoidc-ui/tailwind.config.js
💤 Files with no reviewable changes (1)
- oidc-ui/src/test/components/InputWithImage.test.js
🚧 Files skipped from review as they are similar to previous changes (20)
- oidc-ui/src/test/components/Form.test.js
- oidc-ui/craco.config.js
- oidc-ui/src/test/pages/PageNotFound.test.js
- oidc-ui/src/setupTests.js
- oidc-ui/src/test/commons/ErrorIndicator.test.js
- oidc-ui/src/pages/Consent.js
- oidc-ui/src/App.js
- oidc-ui/src/components/L1Biometrics.js
- oidc-ui/src/components/Authorize.js
- oidc-ui/src/test/components/L1Biometrics.test.js
- oidc-ui/src/common/ErrorIndicator.js
- oidc-ui/src/app/AppRouter.js
- oidc-ui/src/components/Password.js
- oidc-ui/src/test/pages/NetworkError.test.js
- oidc-ui/src/components/IdToken.js
- oidc-ui/src/components/Pin.js
- oidc-ui/src/test/components/Authorize.test.js
- oidc-ui/src/pages/SomethingWrong.js
- oidc-ui/src/test/components/Password.test.js
- oidc-ui/src/test/pages/Login.test.js
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.
Updating react app to 19 version
react-pin-inputwithinput-otpSummary by CodeRabbit
New Features
Refactor
Chores
Tests