You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "luizotavio32 (via GitHub)" <gi...@apache.org> on 2026/07/13 17:27:09 UTC
[PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
luizotavio32 opened a new pull request, #42005:
URL: https://github.com/apache/superset/pull/42005
### SUMMARY
Follow-up to #41942, which replaced the substring `/embedded/` check in `loggerMiddleware` with a regex matching the actual embedded route shapes (`/dashboard/:idOrSlug/embedded/` and `/embedded/:uuid/`).
That regex required a **trailing slash** after the slug/UUID. However, the embedded routes are registered non-strict in React Router (`superset-frontend/src/embedded/index.tsx`), so they also match without a final slash — e.g. `/dashboard/123/embedded` and `/embedded/<uuid>`. For those URLs the regex failed to match, so the events were classified with `source: 'dashboard'` (or omitted) instead of `source: 'embedded_dashboard'`, misreporting embedded-dashboard usage in the event log.
This makes the trailing slash optional in both branches of the regex:
```diff
- /\/dashboard\/[^/]+\/embedded\/|\/embedded\/[^/]+\//;
+ /\/dashboard\/[^/]+\/embedded\/?|\/embedded\/[^/]+\/?/;
```
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — internal event-logging metadata change only.
### TESTING INSTRUCTIONS
Unit tests in `superset-frontend/src/middleware/logger.test.ts` now cover the no-trailing-slash variants:
- `/dashboard/123/embedded` -> `embedded_dashboard`
- `/embedded/abc-def-uuid` -> `embedded_dashboard`
alongside the existing trailing-slash and regular-dashboard cases.
Run:
```bash
cd superset-frontend
npm run test -- src/middleware/logger.test.ts
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4963905005
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4960742404
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4970694625
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4970085384
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4962515077
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42005?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.03%. Comparing base ([`18f1dd3`](https://app.codecov.io/gh/apache/superset/commit/18f1dd394bbeaa11e2eb250a7fb09e367044c22b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6ae4170`](https://app.codecov.io/gh/apache/superset/commit/6ae417068148b0158b688aa9fa336cf38a4d7130?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42005 +/- ##
==========================================
- Coverage 65.03% 65.03% -0.01%
==========================================
Files 2744 2744
Lines 153627 153627
Branches 35226 35226
==========================================
- Hits 99910 99904 -6
- Misses 51810 51816 +6
Partials 1907 1907
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42005/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/42005/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.37% <100.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42005?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #42005:
URL: https://github.com/apache/superset/pull/42005
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4962475883
<!-- Bito Reply -->
The flagged issue is correct. The current regex `//dashboard/[^/]+/embedded/?|/embedded/[^/]+/?/` matches any string containing these patterns, allowing false positives like `/dashboard/123/embeddedXYZ`. To resolve this, you should enforce a segment boundary using a lookahead or by explicitly matching the end of the string or path delimiters.
To fix this, update the regex to require a boundary after the embedded segment:
```typescript
const EMBEDDED_ROUTE_REGEX =
/\/dashboard\/[^/]+\/embedded($|[/?#])|\/embedded\/[^/]+($|[/?#])/;
```
This ensures that `embedded` is followed by the end of the string, a slash, a question mark, or a hash, preventing partial matches on longer segments. There are no other comments on this PR to address.
**superset-frontend/src/middleware/loggerMiddleware.ts**
```
const EMBEDDED_ROUTE_REGEX =
/\/dashboard\/[^/]+\/embedded($|[/?#])|\/embedded\/[^/]+($|[/?#])/;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4961003109
### CI note: playwright failures are unrelated to this change
The `playwright-tests (chromium)` failures are not caused by this PR (which only touches the event-log `source` classification regex in `loggerMiddleware.ts` and its unit test):
- **`clear-all-filters.spec.ts:38`** — flaky: timed out once, then **passed on retry #1**.
- **`delete-display-control.spec.ts:49`** — fails with `locator.hover: Test timeout of 90000ms exceeded`. This is a dashboard display-control hover flow with no relationship to event logging. The spec was added very recently in #41999 and sits on top of the Ant Design v5→v6 upgrade (#41636), which is the base commit of this branch — a hover timeout on a display-control element is consistent with that upgrade / a not-yet-stabilized new test rather than anything here.
`playwright-tests-experimental` passed, and the change is covered by the unit tests in `logger.test.ts`. Happy to rebase once master stabilizes, or a maintainer can re-run the required job.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(event-log): match embedded routes without trailing slash in logger middleware [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42005:
URL: https://github.com/apache/superset/pull/42005#issuecomment-4970087089
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org