codeant-ai-for-open-source[bot] commented on code in PR #39300:
URL: https://github.com/apache/superset/pull/39300#discussion_r3277338350
##########
superset-frontend/playwright.config.ts:
##########
@@ -132,6 +133,26 @@ export default defineConfig({
// No storageState = clean browser with no cached cookies
},
},
+ ...(process.env.INCLUDE_EMBEDDED
+ ? [
+ {
+ // Embedded dashboard tests - validates the full embedding flow:
+ // external app -> SDK -> iframe -> guest token -> dashboard
render.
+ // Each spec file mutates per-dashboard embedding state (UUID,
+ // allowed_domains) on a single shared Superset, so files must not
+ // run in parallel even if more are added later.
+ name: 'chromium-embedded',
+ testMatch: '**/tests/embedded/**/*.spec.ts',
+ fullyParallel: false,
Review Comment:
**Suggestion:** `fullyParallel: false` does not prevent parallel execution
across multiple spec files; it only affects per-file test parallelism. Since
this suite mutates shared embedding state, adding a second embedded spec file
will still run concurrently on multiple workers and create cross-file state
races. Enforce single-worker execution for this project to keep file-level
execution serialized. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded tests can race when multiple spec files exist.
- ⚠️ Flaky failures obscure real regressions in embedding logic.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/playwright.config.ts`, note the global parallelism
settings at
lines 14–16: `fullyParallel: true` and `workers: process.env.CI ? 2 : 1`,
which allow up
to two workers in CI.
2. In the same file, inspect the `chromium-embedded` project at lines 70–83,
where
`testMatch: '**/tests/embedded/**/*.spec.ts'` (line 145) selects embedded
specs and
`fullyParallel: false` at line 146 is intended to prevent problematic
parallelism on
shared embedding state (per the comment at lines 70–74).
3. Add or maintain at least two spec files matching
`**/tests/embedded/**/*.spec.ts` (so
there are multiple files for this project) and run `cd superset-frontend &&
CI=true npx
playwright test --project=chromium-embedded`.
4. Playwright respects `fullyParallel: false` at line 146 only for
test-level parallelism
within a single file; it still schedules different spec files on separate
workers because
`workers` remains `2` globally, so multiple embedded spec files execute
concurrently on
the same shared Superset instance, causing cross-file races on the
per-dashboard embedding
state described in the comment at lines 70–74.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=13250c842d234f5fbe2a825ad175b28a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=13250c842d234f5fbe2a825ad175b28a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright.config.ts
**Line:** 146:146
**Comment:**
*Race Condition: `fullyParallel: false` does not prevent parallel
execution across multiple spec files; it only affects per-file test
parallelism. Since this suite mutates shared embedding state, adding a second
embedded spec file will still run concurrently on multiple workers and create
cross-file state races. Enforce single-worker execution for this project to
keep file-level execution serialized.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=f4515b06b79bad7c964c91e1b40d6d5c58d3a884909cddb0de889e260c1f1f59&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=f4515b06b79bad7c964c91e1b40d6d5c58d3a884909cddb0de889e260c1f1f59&reaction=dislike'>👎</a>
##########
superset-frontend/playwright/helpers/api/embedded.ts:
##########
@@ -0,0 +1,133 @@
+/**
+ * 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 } from '@playwright/test';
+import { apiPost, apiPut } from './requests';
+import { ENDPOINTS as DASHBOARD_ENDPOINTS } from './dashboard';
+
+export const ENDPOINTS = {
+ SECURITY_LOGIN: 'api/v1/security/login',
+ GUEST_TOKEN: 'api/v1/security/guest_token/',
+} as const;
+
+export interface EmbeddedConfig {
+ uuid: string;
+ allowed_domains: string[];
+ dashboard_id: number;
Review Comment:
**Suggestion:** The `EmbeddedConfig` type declares `dashboard_id` as a
number, but the backend response schema exposes it as a string. This mismatch
can cause incorrect assumptions and downstream type/runtime issues when
consumers perform numeric operations. Align the interface with the API
contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Consumers may mis-handle dashboard_id assuming numeric type.
- ⚠️ Future tests risk runtime errors using numeric operations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/playwright/helpers/api/embedded.ts`, inspect the
`EmbeddedConfig`
interface at lines 29–33, which declares `dashboard_id: number;`.
2. In the same file, see `apiEnableEmbedding` at lines 43–55, which calls
the embedded
dashboard endpoint and returns `body.result as EmbeddedConfig`, so all
consumers trust the
`dashboard_id` field to be a `number` at compile time.
3. On the backend, open `superset/superset/dashboards/schemas.py` and inspect
`EmbeddedDashboardResponseSchema` at lines 9–13, which defines `dashboard_id
=
fields.String()`, meaning the API actually returns `dashboard_id` as a JSON
string.
4. When a Playwright test or helper later consumes `EmbeddedConfig` and
performs numeric
operations on `dashboard_id` (for example, `config.dashboard_id.toFixed(0)`
or comparing
it to a numeric ID), TypeScript will compile this based on the declared
`number` type, but
at runtime the value is a string from the API schema, leading to incorrect
assumptions and
potential runtime errors or subtle bugs.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=225eae26397f439586c65c6ba495b843&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=225eae26397f439586c65c6ba495b843&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/helpers/api/embedded.ts
**Line:** 32:32
**Comment:**
*Api Mismatch: The `EmbeddedConfig` type declares `dashboard_id` as a
number, but the backend response schema exposes it as a string. This mismatch
can cause incorrect assumptions and downstream type/runtime issues when
consumers perform numeric operations. Align the interface with the API contract.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=f1b4c83535661b5b4350a646f747ebdd4852fafef52ae218c0df05c938e272f4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=f1b4c83535661b5b4350a646f747ebdd4852fafef52ae218c0df05c938e272f4&reaction=dislike'>👎</a>
##########
superset-frontend/playwright.config.ts:
##########
@@ -132,6 +133,26 @@ export default defineConfig({
// No storageState = clean browser with no cached cookies
},
},
+ ...(process.env.INCLUDE_EMBEDDED
+ ? [
Review Comment:
**Suggestion:** The project gate uses raw string truthiness, so setting
`INCLUDE_EMBEDDED=false` (or `0`) still enables embedded tests because
non-empty strings are truthy in Node. This causes unintended test selection in
CI/local runs. Parse the env var explicitly (for example, only enable when it
equals `'true'`). [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded Playwright project runs despite INCLUDE_EMBEDDED set false.
- ⚠️ Developers cannot reliably disable embedded suite via env.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/playwright.config.ts`, locate the `projects` array
for Playwright
at lines 123–157, and note the embedded project gate
`...(process.env.INCLUDE_EMBEDDED ? [
... ] : [])` at lines 136–155.
2. In a shell, set the environment variable before running tests: `export
INCLUDE_EMBEDDED=false` (a non-empty string) and run `cd superset-frontend
&& npx
playwright test`.
3. When Playwright loads `playwright.config.ts`, Node exposes
`process.env.INCLUDE_EMBEDDED` as the string `'false'`, so the conditional
at line 136
evaluates as truthy and returns the embedded project definition array
instead of `[]`.
4. Observe that the `chromium-embedded` project (lines 144–83) is included
and tests
matching `testMatch: '**/tests/embedded/**/*.spec.ts'` at line 145 are
executed, even
though the flag was set to `'false'`, showing that false-like strings cannot
disable the
project with the current condition.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b014245de0404a49bf6ec31eae8a01f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b014245de0404a49bf6ec31eae8a01f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright.config.ts
**Line:** 136:137
**Comment:**
*Incorrect Condition Logic: The project gate uses raw string
truthiness, so setting `INCLUDE_EMBEDDED=false` (or `0`) still enables embedded
tests because non-empty strings are truthy in Node. This causes unintended test
selection in CI/local runs. Parse the env var explicitly (for example, only
enable when it equals `'true'`).
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=d312da695a32343e4a94e85ffdcce6317802918e0df5771c3be54162a7b1486e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39300&comment_hash=d312da695a32343e4a94e85ffdcce6317802918e0df5771c3be54162a7b1486e&reaction=dislike'>👎</a>
--
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]
