fix(event-log): match embedded routes without trailing slash in logger middleware#42005
fix(event-log): match embedded routes without trailing slash in logger middleware#42005luizotavio32 wants to merge 2 commits into
Conversation
Code Review Agent Run #7ad814Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
CI note: playwright failures are unrelated to this changeThe
|
React Router matches the embedded routes without a trailing slash too, so `/dashboard/:idOrSlug/embedded` and `/embedded/:uuid` are valid embedded URLs. The regex introduced in apache#41942 required a trailing slash, so those events were classified with the wrong source. Make the trailing slash optional in both branches and add test coverage for the no-slash variants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
luizotavio32
force-pushed
the
fix-embedded-route-trailing-slash
branch
from
1cf7e7d to
6ae4170
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current regex To fix this, update the regex to require a boundary after the embedded segment: const EMBEDDED_ROUTE_REGEX =
/\/dashboard\/[^/]+\/embedded($|[/?#])|\/embedded\/[^/]+($|[/?#])/;This ensures that superset-frontend/src/middleware/loggerMiddleware.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42005 +/- ##
=======================================
Coverage 65.03% 65.03%
=======================================
Files 2744 2744
Lines 153627 153627
Branches 35226 35226
=======================================
Hits 99910 99910
Misses 51810 51810
Partials 1907 1907
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The optional trailing slash allowed the first branch to match prefixes like `/dashboard/123/embeddedXYZ`, misclassifying them as embedded. Require an end-of-segment boundary (`/`, `?`, `#`, or end of string) after `embedded` and add test coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #565415Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Follow-up to #41942, which replaced the substring
/embedded/check inloggerMiddlewarewith 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/embeddedand/embedded/<uuid>. For those URLs the regex failed to match, so the events were classified withsource: 'dashboard'(or omitted) instead ofsource: 'embedded_dashboard', misreporting embedded-dashboard usage in the event log.This makes the trailing slash optional in both branches of the regex:
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.tsnow cover the no-trailing-slash variants:/dashboard/123/embedded->embedded_dashboard/embedded/abc-def-uuid->embedded_dashboardalongside the existing trailing-slash and regular-dashboard cases.
Run:
ADDITIONAL INFORMATION