bito-code-review[bot] commented on code in PR #38033:
URL: https://github.com/apache/superset/pull/38033#discussion_r3558282293
##########
superset-frontend/src/pages/Login/Login.test.tsx:
##########
@@ -31,27 +31,26 @@ const defaultBootstrapData = {
},
};
-const mockApplicationRoot = jest.fn<string, []>(() => '');
+jest.mock('src/utils/getBootstrapData', () => ({
+ __esModule: true,
+ default: jest.fn(() => defaultBootstrapData),
+}));
-jest.mock('src/utils/getBootstrapData', () => {
- const actual = jest.requireActual<
- typeof import('src/utils/getBootstrapData')
- >('src/utils/getBootstrapData');
- return {
- __esModule: true,
- ...actual,
- default: jest.fn(() => defaultBootstrapData),
- applicationRoot: () => mockApplicationRoot(),
- };
-});
+const mockEnsureAppRoot = jest.fn((path: string) => path);
+jest.mock('src/utils/pathUtils', () => ({
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mock targets wrong module path</b></div>
<div id="fix">
The mock targets `src/utils/pathUtils` but `Login/index.tsx:37` imports
`ensureAppRoot` from `src/utils/navigationUtils` — these are two distinct
modules. The mock will not intercept the component's import, causing
`ensureAppRoot` calls to use the real implementation instead of the mock. This
undermines all 13 `mockEnsureAppRoot` assertions in the diff.
</div>
</div>
<small><i>Code Review Run #6e5d9b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -96,15 +96,19 @@ export default function Login() {
}, []);
const loginEndpoint = useMemo(
- () => (nextUrl ? `/login/?next=${encodeURIComponent(nextUrl)}` :
'/login/'),
+ () =>
+ ensureAppRoot(
+ nextUrl ? `/login/?next=${encodeURIComponent(nextUrl)}` : '/login/',
+ ),
[nextUrl],
);
const buildProviderLoginUrl = (providerName: string) => {
- const base = `/login/${encodeURIComponent(providerName)}`;
- return ensureAppRoot(
- nextUrl ? `${base}?next=${encodeURIComponent(nextUrl)}` : base,
- );
+ const base = `/login/${providerName}`;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing URL encoding regression</b></div>
<div id="fix">
The change removes `encodeURIComponent(providerName)` which introduces a
behavioral regression. Provider names with spaces or special characters (e.g.,
'Okta SSO', 'Test&Dev') will produce malformed URLs. Compare line 109 which
correctly encodes `nextUrl` — `providerName` encoding should be consistent with
this pattern. This aligns with how the original code worked. (See also:
[CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #6e5d9b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
