codeant-ai-for-open-source[bot] commented on code in PR #42005:
URL: https://github.com/apache/superset/pull/42005#discussion_r3573995430
##########
superset-frontend/src/middleware/loggerMiddleware.ts:
##########
@@ -136,7 +136,7 @@ const logMessageQueue = new
DebouncedMessageQueue<LogEventData>({
// `/embedded/:uuid/`. Matching these specific shapes avoids false positives
// for regular dashboards (e.g. a dashboard whose slug is "embedded").
const EMBEDDED_ROUTE_REGEX =
- /\/dashboard\/[^/]+\/embedded\/|\/embedded\/[^/]+\//;
+ /\/dashboard\/[^/]+\/embedded\/?|\/embedded\/[^/]+\/?/;
Review Comment:
**Suggestion:** The updated regex makes the trailing slash optional but does
not enforce a segment boundary after `embedded`, so it will also match paths
like `/dashboard/123/embeddedXYZ` or `/embedded/uuidExtra` and incorrectly
classify them as embedded dashboards. Require an end-of-segment boundary after
`embedded`/UUID (for example end of string, `?`, `#`, or `/`) to keep matching
aligned with real route shapes. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded usage metrics may include non-embedded URLs.
- ⚠️ Event log misclassifies some dashboard paths as embedded.
- ⚠️ Future routes prefixed with /embedded risk misreporting.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/embedded/index.tsx` at lines 142-143, note that
the embedded
routes are explicitly registered as `/dashboard/:idOrSlug/embedded/` and
`/embedded/:uuid/`, i.e. the `embedded` segment must be the final path
segment for
embedded dashboards.
2. In `superset-frontend/src/middleware/loggerMiddleware.ts` at lines
135-139, observe
`EMBEDDED_ROUTE_REGEX =
/\/dashboard\/[^/]+\/embedded\/?|\/embedded\/[^/]+\/?/;`, and at
lines 172-176 see `const isEmbedded = EMBEDDED_ROUTE_REGEX.test(path ??
'');` followed by
`source: isEmbedded ? 'embedded_dashboard' : 'dashboard'` in the log
metadata.
3. In `superset-frontend/src/middleware/logger.test.ts` at lines 140-149,
use the existing
`getSourceForPath` helper to simulate navigation with a custom URL: call
`getSourceForPath('http://localhost/dashboard/123/embeddedXYZ')`, which sets
`window.location.href` to this value and then invokes `loggerMiddleware`.
4. When this test runs, `EMBEDDED_ROUTE_REGEX` matches the substring
`/dashboard/123/embedded` inside `/dashboard/123/embeddedXYZ` (because
`/embedded\/?` does
not enforce a segment boundary), `isEmbedded` becomes `true`, and the
emitted event at
`loggerMiddleware.ts:176-178` is classified with `source:
'embedded_dashboard'` even
though the route shape `/dashboard/123/embeddedXYZ` does not correspond to
the real
embedded routes defined in `embedded/index.tsx`, demonstrating the overly
permissive
matching.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9682a3385e4a4f3a8896ff8a34466dba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9682a3385e4a4f3a8896ff8a34466dba&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/src/middleware/loggerMiddleware.ts
**Line:** 139:139
**Comment:**
*Incorrect Condition Logic: The updated regex makes the trailing slash
optional but does not enforce a segment boundary after `embedded`, so it will
also match paths like `/dashboard/123/embeddedXYZ` or `/embedded/uuidExtra` and
incorrectly classify them as embedded dashboards. Require an end-of-segment
boundary after `embedded`/UUID (for example end of string, `?`, `#`, or `/`) to
keep matching aligned with real route shapes.
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%2F42005&comment_hash=d17aea4175592b1028e2040b921cf8584908a8d4c6006847b47bf60ab524218b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42005&comment_hash=d17aea4175592b1028e2040b921cf8584908a8d4c6006847b47bf60ab524218b&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]
