fix(dashboard): persist native filter state when reopening dashboard - #43085
fix(dashboard): persist native filter state when reopening dashboard#43085sunny-singh78277 wants to merge 1 commit into
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!id || hydratedDashboardId !== id) return; | ||
| saveDashboardFilters(id, fullDataMask); | ||
| }, [id, hydratedDashboardId, fullDataMask]); |
There was a problem hiding this comment.
Suggestion: The persistence effect runs before the hydration effect and can overwrite the saved state with the current Redux mask, commonly {} during the initial render. When the hydration effect subsequently reads localStorage, it restores the already-erased value instead of the user's filters. Defer persistence until after dashboard hydration has completed, or prevent the initial write until the saved state has been loaded. [stale reference]
Severity Level: Major ⚠️
- ❌ Reopening dashboards can lose persisted native-filter selections.
- ⚠️ SPA dashboard reuse can overwrite saved filter state.
- ⚠️ Filter selections must be manually restored by users.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 189:192
**Comment:**
*Stale Reference: The persistence effect runs before the hydration effect and can overwrite the saved state with the current Redux mask, commonly `{}` during the initial render. When the hydration effect subsequently reads localStorage, it restores the already-erased value instead of the user's filters. Defer persistence until after dashboard hydration has completed, or prevent the initial write until the saved state has been loaded.
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| } else { | ||
| const savedFilters = getSavedDashboardFilters(id); | ||
| if (savedFilters) { | ||
| dataMask = savedFilters; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The saved filter lookup is inside an effect that only runs when readyToRender changes. During SPA navigation, the component can remain ready while id and the fetched dashboard data change, so this branch is skipped for the new dashboard and its saved filters are never hydrated. Include the dashboard identity in the hydration trigger and reset the hydration lifecycle when navigating between dashboards. [state/lifecycle]
Severity Level: Major ⚠️
- ❌ Dashboard-to-dashboard navigation skips saved native filters.
- ⚠️ Dashboard B can display stale filter state from dashboard A.
- ⚠️ Users lose expected filter persistence during SPA navigation.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 259:264
**Comment:**
*State Lifecycle: The saved filter lookup is inside an effect that only runs when `readyToRender` changes. During SPA navigation, the component can remain ready while `id` and the fetched dashboard data change, so this branch is skipped for the new dashboard and its saved filters are never hydrated. Include the dashboard identity in the hydration trigger and reset the hydration lifecycle when navigating between dashboards.
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #732b46
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/containers/DashboardPage.tsx - 1
- Missing unit test coverage · Line 86-108
Review Details
-
Files reviewed - 1 · Commit Range:
9c08cc6..9c08cc6- superset-frontend/src/dashboard/containers/DashboardPage.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_'; | ||
|
|
||
| function getSavedDashboardFilters(dashboardId: number) { | ||
| try { | ||
| const raw = localStorage.getItem( | ||
| `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`, | ||
| ); | ||
| return raw ? JSON.parse(raw) : null; | ||
| } catch { | ||
| return null; // localStorage disabled, quota exceeded, corrupt JSON, etc. | ||
| } | ||
| } | ||
|
|
||
| function saveDashboardFilters(dashboardId: number, dataMask: unknown) { | ||
| try { | ||
| localStorage.setItem( | ||
| `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`, | ||
| JSON.stringify(dataMask), | ||
| ); | ||
| } catch { | ||
| // fail silently — persistence is a nice-to-have, not critical path | ||
| } | ||
| } |
There was a problem hiding this comment.
New localStorage persistence functions lack unit tests per BITO.md adaptive rule [6262]. Tests should verify: (1) JSON parse error returns null, (2) localStorage disabled returns null, (3) correct key prefix construction, (4) successful save roundtrip.
Code Review Run #732b46
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43085 +/- ##
=======================================
Coverage 66.64% 66.64%
=======================================
Files 2863 2863
Lines 162112 162129 +17
Branches 37384 37388 +4
=======================================
+ Hits 108033 108049 +16
- Misses 52021 52022 +1
Partials 2058 2058
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:
|
1 participant
SUMMARY
Native filter selections are currently lost whenever a user navigates away
from a dashboard and returns via normal UI (dashboard list, nav menu, a new
tab) without the exact
native_filters_key/permalink_keyURL param —filters revert to configured defaults or empty.
This adds a localStorage fallback: the dashboard's dataMask is saved per
dashboard whenever it changes, and restored on load only when no filter key
is present in the URL. The save is gated on the currently-hydrated dashboard
id matching the dashboard being viewed, to prevent one dashboard's filter
state leaking into another's storage key during in-app navigation between
dashboards.
Note: a maintainer raised on the linked issue that dashboard filters are
intended to be ephemeral by design, with persistence handled via saved filter
defaults or shared permalink URLs. This PR addresses a narrower case — a
single user losing their own just-applied filters within their own session,
without any sharing intent — open for discussion on whether this should be
default-on (as implemented here) or opt-in.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
[attach: apply filters, reopen via dashboard list, filters shown restored]
TESTING INSTRUCTIONS
the same dashboard.
reload) and confirm B does not inherit A's filter state.
ADDITIONAL INFORMATION