-
Notifications
You must be signed in to change notification settings - Fork 16.5k
fix(log): store navigation path to get correct logging path #32795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(log): store navigation path to get correct logging path #32795
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
| Global Mutable State ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/middleware/loggerMiddleware.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| let lastEventId = 0; | ||
| const loggerMiddleware = store => next => action => { | ||
| if (action.type !== LOG_EVENT) { | ||
| return next(action); | ||
| } | ||
| const loggerMiddleware = store => next => { | ||
| let navPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Global Mutable State 
Tell me more
What is the issue?
The middleware uses mutable global state (lastEventId and effectively navPath) which makes the code harder to reason about and test.
Why this matters
Global mutable state can lead to unexpected behavior, makes testing difficult, and violates principles of functional programming that Redux typically encourages.
Suggested change ∙ Feature Preview
Encapsulate the state within a closure or class:
const createLoggerMiddleware = () => {
let lastEventId = 0;
let navPath = null;
return store => next => action => {
// middleware logic
};
};
export default createLoggerMiddleware();Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
(cherry picked from commit 4a70065)
(cherry picked from commit 4a70065)
(cherry picked from commit 4a70065)
(cherry picked from commit 4a70065)
(cherry picked from commit 4a70065)
(cherry picked from commit 4a70065)
* chore: bump base image in Dockerfile with `ARG PY_VER=3.11.11-slim-bookworm` (apache#32780) * chore: Revert "chore: bump base image in Dockerfile with `ARG PY_VER=3.11.11-slim-bookworm`" (apache#32782) * fix(chart data): removing query from /chart/data payload when accessing as guest user (apache#30858) (cherry picked from commit dd39138) * fix: upgrade to 3.11.11-slim-bookworm to address critical vulnerabilities (apache#32240) (cherry picked from commit ad05732) * fix(model/helper): represent RLS filter clause in proper textual SQL string (apache#32406) Signed-off-by: hainenber <[email protected]> (cherry picked from commit ff0529c) * fix: Log table retention policy (apache#32572) (cherry picked from commit 89b6d7f) * fix(welcome): perf on distinct recent activities (apache#32608) (cherry picked from commit 832e028) * fix(log): Update recent_activity by event name (apache#32681) (cherry picked from commit 449f51a) * fix: Signature of Celery pruner jobs (apache#32699) (cherry picked from commit df06bdf) * fix(logging): missing path in event data (apache#32708) (cherry picked from commit cd5a943) * fix(fe/dashboard-list): display modifier info for `Last modified` data (apache#32035) Signed-off-by: hainenber <[email protected]> (cherry picked from commit 88cf2d5) * fix: make packages PEP 625 compliant (apache#32866) Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 6e02d19) * all cccs changes * fix: Downgrade to marshmallow<4 (apache#33216) * fix(log): store navigation path to get correct logging path (apache#32795) (cherry picked from commit 4a70065) * fix(pivot-table): Revert "fix(Pivot Table): Fix column width to respect currency config (apache#31414)" (apache#32968) (cherry picked from commit a36e636) * fix: improve error type on parse error (apache#33048) (cherry picked from commit ed0cd5e) * fix(plugin-chart-echarts): remove erroneous upper bound value (apache#32473) (cherry picked from commit 5766c36) * fix(pinot): revert join and subquery flags (apache#32382) (cherry picked from commit 822d72c) * fix: loading examples from raw.githubusercontent.com fails with 429 errors (apache#33354) (cherry picked from commit f045a73) * chore: creating 4.1.3rc1 change log and updating frontend json (cherry picked from commit 72cf9b6) * chore(🦾): bump python sqlglot 26.1.3 -> 26.11.1 (apache#32745) Co-authored-by: GitHub Action <[email protected]> (cherry picked from commit 66c1a6a) * chore(🦾): bump python h11 0.14.0 -> 0.16.0 (apache#33339) Co-authored-by: GitHub Action <[email protected]> (cherry picked from commit 8252686) * docs: CVEs fixed on 4.1.2 (apache#33435) (cherry picked from commit 8a8fb49) * feat(api): Added uuid to list api calls (apache#32414) (cherry picked from commit 8decc9e) * fix(table-chart): time shift is not working (apache#33425) (cherry picked from commit dc44748) * fix(Sqllab): Autocomplete got stuck in UI when open it too fast (apache#33522) (cherry picked from commit b4e2406) * chore: update Dockerfile - Upgrade to 3.11.12 (apache#33612) (cherry picked from commit f0b6e87) * chore: updating 4.1.3rc2 change log * Select all Drag and Drop (#546) * add a select all button for the dnd select * remove cypress * chore(deps): bump cryptography from 43.0.3 to 44.0.1 (apache#32236) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit fa09d81) * fix: Adds missing __init__ file to commands/logs (apache#33059) (cherry picked from commit c1159c5) * fix: Saved queries list break if one query can't be parsed (apache#34289) (cherry picked from commit 1e5a4e9) * chore: Adds 4.1.4RC1 data to CHANGELOG.md and UPDATING.md * tag bump for select all drag and drop * Fix package-lock.json * Add db migration, bump Docker image base * gevent for gunicorn * remove threads and make worker-connections configurable * Fix package-lock.json * tag bump for cccs build * Remove CCCS Dataset Explorer (#550) * tag bump for CCCS build --------- Signed-off-by: hainenber <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: gpchandran <[email protected]> Co-authored-by: Joe Li <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: Đỗ Trọng Hải <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: JUST.in DO IT <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: Andreas Motl <[email protected]> Co-authored-by: Ville Brofeldt <[email protected]> Co-authored-by: Yuri <[email protected]> Co-authored-by: Maxime Beauchemin <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <[email protected]> Co-authored-by: sha174n <[email protected]> Co-authored-by: Paul Rhodes <[email protected]> Co-authored-by: Rafael Benitez <[email protected]> Co-authored-by: cccs-RyanK <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: cyber-jessie <[email protected]>
SUMMARY
To determine the event source of logging, the previous patch referenced the current path using
window.location. However, when navigating quickly between pages, the logging from the previous page was being processed, resulting in the event source of the previous page's logging being recorded with the current page's URL.To resolve this issue, this commit modified the process to log the path at the time the
spa_navigationevent occurs, ensuring that the records during navigation are correctly attributed to the appropriate source.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:

After:

TESTING INSTRUCTIONS
specs and screenshots
ADDITIONAL INFORMATION