codeant-ai-for-open-source[bot] commented on code in PR #36196:
URL: https://github.com/apache/superset/pull/36196#discussion_r2583639439
##########
superset-frontend/playwright/components/core/index.ts:
##########
@@ -21,3 +21,5 @@
export { Button } from './Button';
export { Form } from './Form';
export { Input } from './Input';
+export { Modal } from './Modal';
Review Comment:
**Suggestion:** The new export for the Modal component points to './Modal',
but there is no corresponding Modal module in this directory, so any attempt to
build or run the Playwright tests that import from this core index will fail
with a "module not found" error at build time. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The export targets './Modal', but a glob search under
'superset-frontend/playwright/components/core/' shows no Modal module,
so this will cause a module resolution/compile-time error when the
index is imported. Removing the export (as implied by the empty
improved_code) would eliminate the immediate build break, so the
suggestion addresses a real correctness issue rather than mere style.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/components/core/index.ts
**Line:** 24:24
**Comment:**
*Possible Bug: The new export for the Modal component points to
'./Modal', but there is no corresponding Modal module in this directory, so any
attempt to build or run the Playwright tests that import from this core index
will fail with a "module not found" error at build time.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/playwright/pages/AuthPage.ts:
##########
@@ -83,6 +84,62 @@ export class AuthPage {
await loginButton.click();
}
+ /**
+ * Wait for successful login by verifying the login response and session
cookie.
+ * Call this after loginWithCredentials to ensure authentication completed.
+ *
+ * This does NOT assume a specific landing page (which is configurable).
+ * Instead it:
+ * 1. Checks if session cookie already exists (guards against race condition)
+ * 2. Waits for POST /login/ response with redirect status
+ * 3. Polls for session cookie to appear
+ *
+ * @param options - Optional wait options
+ */
+ async waitForLoginSuccess(options?: { timeout?: number }): Promise<void> {
+ const timeout = options?.timeout || TIMEOUT.PAGE_LOAD;
Review Comment:
**Suggestion:** The `timeout` option in `waitForLoginSuccess` is using `||`
instead of nullish coalescing, so a valid timeout value of `0` (or any other
falsy value) will be ignored and replaced with the default, which means callers
cannot reliably control the timeout and may see longer waits than requested.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
const timeout = options?.timeout ?? TIMEOUT.PAGE_LOAD;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The criticism is correct: using `||` means a caller cannot set a legitimate
timeout of `0` (or any other falsy non-undefined value), because it will be
replaced with `TIMEOUT.PAGE_LOAD`. That's a real logic bug in how the
API handles its own `options.timeout`, not just style. Switching to `??`
correctly preserves all values except `null`/`undefined`, fixing the
semantics without breaking any current usage (nothing in the repo passes
a timeout today).
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/pages/AuthPage.ts
**Line:** 100:100
**Comment:**
*Logic Error: The `timeout` option in `waitForLoginSuccess` is using
`||` instead of nullish coalescing, so a valid timeout value of `0` (or any
other falsy value) will be ignored and replaced with the default, which means
callers cannot reliably control the timeout and may see longer waits than
requested.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/playwright/pages/AuthPage.ts:
##########
@@ -83,6 +84,62 @@ export class AuthPage {
await loginButton.click();
}
+ /**
+ * Wait for successful login by verifying the login response and session
cookie.
+ * Call this after loginWithCredentials to ensure authentication completed.
+ *
+ * This does NOT assume a specific landing page (which is configurable).
+ * Instead it:
+ * 1. Checks if session cookie already exists (guards against race condition)
+ * 2. Waits for POST /login/ response with redirect status
+ * 3. Polls for session cookie to appear
+ *
+ * @param options - Optional wait options
+ */
+ async waitForLoginSuccess(options?: { timeout?: number }): Promise<void> {
+ const timeout = options?.timeout || TIMEOUT.PAGE_LOAD;
+ const startTime = Date.now();
+
+ // 1. Guard: Check if session cookie already exists (race condition
protection)
+ const existingCookie = await this.getSessionCookie();
+ if (existingCookie?.value) {
+ // Already authenticated - login completed before we started waiting
+ return;
+ }
+
+ // 2. Wait for POST /login/ response
+ const loginResponse = await this.waitForLoginRequest();
+
Review Comment:
**Suggestion:** In `waitForLoginSuccess`, the explicit timeout is only
applied to the cookie polling loop and not to the initial `waitForResponse` on
the login POST, so if the login request never occurs or hangs, this call can
block for Playwright's default timeout plus your configured timeout instead of
respecting the requested overall limit; passing the timeout through to
`waitForResponse` fixes this. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const loginResponse = await this.page.waitForResponse(
response =>
response.url().includes('/login/') &&
response.request().method() === 'POST',
{ timeout },
);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Right now `waitForLoginSuccess` computes its own `timeout` but then calls
`waitForLoginRequest()`, which in turn relies on Playwright's default
timeout instead of the caller's. Only the cookie polling loop is bounded
by the provided `timeout`, so the total wait can be "Playwright default
for the response + our timeout", violating the caller's expectation of a
single upper bound. Inlining `page.waitForResponse` here and passing
`{ timeout }` makes this method honor the configured timeout end-to-end.
The code is syntactically fine with existing imports and doesn't break
other usages of `waitForLoginRequest`.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/pages/AuthPage.ts
**Line:** 112:112
**Comment:**
*Possible Bug: In `waitForLoginSuccess`, the explicit timeout is only
applied to the cookie polling loop and not to the initial `waitForResponse` on
the login POST, so if the login request never occurs or hangs, this call can
block for Playwright's default timeout plus your configured timeout instead of
respecting the requested overall limit; passing the timeout through to
`waitForResponse` fixes this.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/playwright/tests/auth/login.spec.ts:
##########
@@ -20,69 +20,74 @@
import { test, expect } from '@playwright/test';
import { AuthPage } from '../../pages/AuthPage';
import { URL } from '../../utils/urls';
+import { TIMEOUT } from '../../utils/constants';
-test.describe('Login view', () => {
- let authPage: AuthPage;
+// Test credentials - can be overridden via environment variables
+const adminUsername = process.env.PLAYWRIGHT_ADMIN_USERNAME || 'admin';
+const adminPassword = process.env.PLAYWRIGHT_ADMIN_PASSWORD || 'general';
- test.beforeEach(async ({ page }: any) => {
- authPage = new AuthPage(page);
- await authPage.goto();
- await authPage.waitForLoginForm();
- });
+/**
+ * Auth/login tests use per-test navigation via beforeEach.
+ * Each test starts fresh on the login page without global authentication.
+ * This follows the Cypress pattern for auth testing - simple and isolated.
+ */
- test('should redirect to login with incorrect username and password', async
({
- page,
- }: any) => {
- // Setup request interception before login attempt
- const loginRequestPromise = authPage.waitForLoginRequest();
+let authPage: AuthPage;
- // Attempt login with incorrect credentials
- await authPage.loginWithCredentials('admin', 'wrongpassword');
+test.beforeEach(async ({ page }) => {
+ // Navigate to login page before each test (ensures clean state)
+ authPage = new AuthPage(page);
+ await authPage.goto();
+ await authPage.waitForLoginForm();
+});
- // Wait for login request and verify response
- const loginResponse = await loginRequestPromise;
- // Failed login returns 401 Unauthorized or 302 redirect to login
- expect([401, 302]).toContain(loginResponse.status());
+test('should redirect to login with incorrect username and password', async ({
+ page,
+}) => {
+ // Setup request interception before login attempt
+ const loginRequestPromise = authPage.waitForLoginRequest();
- // Wait for redirect to complete before checking URL
- await page.waitForURL((url: any) => url.pathname.endsWith('login/'), {
- timeout: 10000,
- });
+ // Attempt login with incorrect credentials
+ await authPage.loginWithCredentials(adminUsername, 'wrongpassword');
Review Comment:
**Suggestion:** The negative-login test description says it should use an
incorrect username and password, but the implementation uses the (potentially
valid) admin username with only an incorrect password, so the test does not
actually cover the "incorrect username" scenario and can give a false sense of
coverage for that case. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
await authPage.loginWithCredentials('wronguser', 'wrongpassword');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test description explicitly states it covers "incorrect username and
password", but the current code only uses an incorrect password with a valid
username. That's a real logical mismatch: the scenario implied by the test name
and comment (invalid username) is not exercised, so coverage for that case is
missing and the test is misleading. Changing the username argument to a clearly
invalid value makes the test assert what it claims to, improving the
reliability of the test suite rather than being a mere cosmetic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/tests/auth/login.spec.ts
**Line:** 51:51
**Comment:**
*Logic Error: The negative-login test description says it should use an
incorrect username and password, but the implementation uses the (potentially
valid) admin username with only an incorrect password, so the test does not
actually cover the "incorrect username" scenario and can give a false sense of
coverage for that case.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/playwright/pages/ExplorePage.ts:
##########
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Page, Locator } from '@playwright/test';
+import { TIMEOUT } from '../utils/constants';
+
+/**
+ * Explore Page object
+ */
+export class ExplorePage {
+ private readonly page: Page;
+
+ private static readonly SELECTORS = {
+ DATASOURCE_CONTROL: '[data-test="datasource-control"]',
+ VIZ_SWITCHER: '[data-test="fast-viz-switcher"]',
+ } as const;
+
+ constructor(page: Page) {
+ this.page = page;
+ }
+
+ /**
+ * Waits for the Explore page to load.
+ * Validates URL contains /explore/ and datasource control is visible.
+ *
+ * @param options - Optional wait options
+ */
+ async waitForPageLoad(options?: { timeout?: number }): Promise<void> {
+ const timeout = options?.timeout || TIMEOUT.PAGE_LOAD;
Review Comment:
**Suggestion:** The timeout fallback uses the `||` operator, which treats
`0` as falsy and overrides it with the default, preventing callers from
explicitly disabling or setting a zero timeout as supported by Playwright, so
it should use nullish coalescing to only fall back when the value is `null` or
`undefined`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const timeout = options?.timeout ?? TIMEOUT.PAGE_LOAD;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code treats `0` as falsy and replaces it with
`TIMEOUT.PAGE_LOAD`,
which is a real semantic bug because Playwright allows `timeout: 0` to mean
"no timeout/disabled timeout." Changing this to use nullish coalescing (`??`)
preserves valid `0` values while still falling back for `undefined`/`null`.
This is not cosmetic; it corrects broken behavior for a legitimate parameter
value even if current callers don't yet pass `0`.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/pages/ExplorePage.ts
**Line:** 45:45
**Comment:**
*Logic Error: The timeout fallback uses the `||` operator, which treats
`0` as falsy and overrides it with the default, preventing callers from
explicitly disabling or setting a zero timeout as supported by Playwright, so
it should use nullish coalescing to only fall back when the value is `null` or
`undefined`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
