You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "omsn2 (via GitHub)" <gi...@apache.org> on 2026/08/13 08:55:38 UTC
[PR] feat(dashboard): persist native filter state to localStorage across navigation [superset]
omsn2 opened a new pull request, #43108:
URL: https://github.com/apache/superset/pull/43108
### SUMMARY
Native filter selections were lost whenever users navigated away from a dashboard
and returned via normal Superset navigation (menu, dashboard list, bookmarks, fresh tab).
Filter state was only recoverable if the exact URL containing `native_filters_key`
was manually preserved — making repeated daily analysis frustrating.
This fix adds lightweight client-side persistence using `localStorage`:
- On every filter change, saves the full `dataMask` to `localStorage` keyed by `dashboardId`
- On dashboard load, if no `permalinkKey` or `nativeFiltersKey` is present in the URL,
falls back to the last saved state from `localStorage`
- Uses `hydratedDashboardId === id` guard (Redux-driven) instead of a `useRef` flag to
prevent writing one dashboard's filter state under another dashboard's key during
SPA navigation between dashboards
Fixes #43057
### BEFORE / AFTER SCREENSHOTS
**Before:** Filters reset to configured defaults every time a user navigated away
and returned via the normal Superset UI.
**After:** Filters are automatically restored from the user's last applied state on
every subsequent visit, without requiring URL preservation.
### TESTING INSTRUCTIONS
1. Open any dashboard that has native filters configured
2. Apply one or more filter values (e.g. select a country, date range, etc.)
3. Navigate away via the top Superset menu (e.g. Dashboards list)
4. Click back into the same dashboard from the list
5. ✅ Verify: filters are restored to the previously applied values automatically
### ADDITIONAL INFORMATION
**Known limitations (acknowledged in the linked issue):**
- Per-browser only — does not sync across devices or browsers
- No per-user scoping — on a shared browser profile, one user's filters may
appear for another user of the same browser
- No TTL or invalidation — stale saved filters may persist if an admin
changes filter configuration or defaults
A server-side approach (e.g. `last_filter_state` per user+dashboard) would address
these gaps but is out of scope for this lightweight client-side fix.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3777573415
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+) {
+ try {
+ localStorage.setItem(
+ getStorageKey(dashboardId, userId),
+ JSON.stringify(nativeFilterMask),
+ );
+ } catch {
Review Comment:
`saveDashboardFilters` writes to localStorage on every relevant state change; `localStorage.setItem` is synchronous and can become a noticeable main-thread cost. You can avoid redundant writes by comparing the serialized value to the existing stored value before setting it.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
Review Comment:
New localStorage keys are expected to be namespaced using the `namespace__key` pattern (see `src/utils/localStorageHelpers.ts`), but this new key uses a single-underscore prefix. Using the shared convention reduces collision risk and keeps keys consistent across the app.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +267,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
`getSavedDashboardFilters()` returns `any` from `JSON.parse`, and the result is assigned directly to `dataMask`. If localStorage is corrupted/tampered (or contains a non-object), this can break downstream code that assumes `dataMask` is an object map. Guard the shape before using it (and optionally filter keys to configured native filters).
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +427,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
Review Comment:
This change introduces new behavior (persisting/restoring native filter state from localStorage, including per-user scoping and the hydration guard), but there are no unit tests covering it. Adding tests around the load path (no URL key => restores) and save path (only native filter ids persisted, userId scoping) would prevent regressions.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
Review Comment:
The PR description lists "No per-user scoping" as a known limitation, but this implementation *does* scope by `userId` when available. Please update the PR description (and/or linked issue notes) so behavior and documented limitations match.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4129011476
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +410,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
Review Comment:
The concern is valid. `nativeFilters` includes dividers and chart customizations, so using `Object.keys(nativeFilters)` can still serialize non-native entries whenever they exist in `fullDataMask`. Although restore later drops them, this causes unnecessary writes and storage serialization.
The save path should derive its allowlist from the dashboard’s actual `native_filter_configuration`, matching the restore validation:
```ts
const nativeFilterIds = new Set(
(
(dashboard?.metadata?.native_filter_configuration ?? []) as Array<
NativeFilterConfigEntry | null | undefined
>
)
.filter(
(filter): filter is NativeFilterConfigEntry =>
filter != null && !!filter.id,
)
.map(filter => filter.id),
);
const nativeFilterMask = Object.fromEntries(
Object.entries(fullDataMask).filter(
([filterId]) => nativeFilterIds.has(filterId),
),
);
if (Object.keys(nativeFilterMask).length === 0) return;
saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
```
This ensures chart customizations, dividers, and synthetic/transient entries are excluded before serialization, rather than merely being discarded during restore. The change is necessary to keep persistence limited to the feature’s intended native-filter state and avoid redundant writes.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5955287744
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4174691773
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,628 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ const FILTER_ID = 'NATIVE_FILTER-guest';
+ const filterConfig = {
+ id: FILTER_ID,
+ name: 'Team',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 1, column: { name: 'col' } }],
+ defaultDataMask: {},
+ };
+
+ // Give the dashboard a matching native filter definition so that it would be restored if not a guest
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [filterConfig],
+ },
+ json_metadata: JSON.stringify({
+ native_filter_configuration: [filterConfig],
+ }),
+ },
+ error: null,
+ });
+
+ const nativeFilters = {
+ filters: {
+ [FILTER_ID]: filterConfig,
+ },
+ };
+
+ const savedVersioned = {
+ dataMask: {
+ [FILTER_ID]: {
+ id: FILTER_ID,
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ [FILTER_ID]: {
+ id: FILTER_ID,
+ targets: filterConfig.targets,
+ type: filterConfig.filterType,
+ defaultDataMask: filterConfig.defaultDataMask,
+ },
+ },
+ };
+
+ // Write under the guest (dashboard-only) key
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: {
+ id: 1,
+ metadata: { native_filter_configuration: [filterConfig] },
+ },
+ dashboardState: { sliceIds: [] },
+ nativeFilters,
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ window.localStorage.clear();
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-670: Vacuous negative assertion</b></div>
<div id="fix">
The negative assertion `not.toHaveBeenCalledWith` also passes if `hydrateDashboard` is never called in scenario 2, so the test can pass without exercising the target-mismatch drop logic in `getDataMaskApplied`. Add a positive control first (e.g. `toHaveBeenCalledWith(expect.objectContaining({ dataMask: {} }))`) to prove hydration ran. ([CWE-670](https://cwe.mitre.org/data/definitions/670.html))
</div>
</div>
<small><i>Code Review Run #c42069</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,628 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ const FILTER_ID = 'NATIVE_FILTER-guest';
+ const filterConfig = {
+ id: FILTER_ID,
+ name: 'Team',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 1, column: { name: 'col' } }],
+ defaultDataMask: {},
+ };
+
+ // Give the dashboard a matching native filter definition so that it would be restored if not a guest
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [filterConfig],
+ },
+ json_metadata: JSON.stringify({
+ native_filter_configuration: [filterConfig],
+ }),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead mock fixture field</b></div>
<div id="fix">
The `json_metadata` field set on the `useDashboard` mock result is never consumed: `DashboardPage.tsx` reads filter config from `dashboard.metadata.native_filter_configuration`, and `hydrateDashboard` is mocked in this suite. This dead setup implies `json_metadata` drives behavior when it does not — remove it to keep the fixture minimal.
</div>
</div>
<small><i>Code Review Run #c42069</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,628 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ const FILTER_ID = 'NATIVE_FILTER-guest';
+ const filterConfig = {
+ id: FILTER_ID,
+ name: 'Team',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 1, column: { name: 'col' } }],
+ defaultDataMask: {},
+ };
+
+ // Give the dashboard a matching native filter definition so that it would be restored if not a guest
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [filterConfig],
+ },
+ json_metadata: JSON.stringify({
+ native_filter_configuration: [filterConfig],
+ }),
+ },
+ error: null,
+ });
+
+ const nativeFilters = {
+ filters: {
+ [FILTER_ID]: filterConfig,
+ },
+ };
+
+ const savedVersioned = {
+ dataMask: {
+ [FILTER_ID]: {
+ id: FILTER_ID,
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ [FILTER_ID]: {
+ id: FILTER_ID,
+ targets: filterConfig.targets,
+ type: filterConfig.filterType,
+ defaultDataMask: filterConfig.defaultDataMask,
+ },
+ },
+ };
+
+ // Write under the guest (dashboard-only) key
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: {
+ id: 1,
+ metadata: { native_filter_configuration: [filterConfig] },
+ },
+ dashboardState: { sliceIds: [] },
+ nativeFilters,
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ window.localStorage.clear();
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant requireActual render</b></div>
<div id="fix">
`jest.requireActual('spec/helpers/testing-library')` re-resolves the same module already imported at the top of the file (no `jest.mock` targets it here), and the second mount at line 1041 uses the top-level `render`. Two different `render` references for the same operation is redundant and misleading — use the imported `render` for both.
</div>
</div>
<small><i>Code Review Run #c42069</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3888668503
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
Fixed in the latest commit. I added an !isDashboardHydrated.current check to the save effect's early return guard. This guarantees that saveDashboardFilters explicitly waits until the current dashboard has fully completed its restore/hydration process before it's allowed to persist anything back to localStorage. This eliminates the edge case where a cleared dataMask could persist {} prior to restoration.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5470873068
Tagging @kgabryje @michael-s-molina and @villebro on this one, since they might have the most institutional memory around native filter state/storage, and remember reasons we didn't do this before. Seems harmless as long as it's non-breaking for others dpeending on filter state persisted in the key/value store and retrieved via the filter key in the URL. If this is just additive like caching, it's probably a nice addition.
Without having looked at the code yet, I'll note "some does it/should it" questions:
• If a dashboard is loaded with no url filter key, and it applies the filters, does/should it ADD the filter key to the URL via push state? One of the benefits of having it there is that you can share that URL with others, so they can benefit from that filter config. Also, just a "truthiness" thing to show more accurately that these filters are applied
• If someone sends you a URL with a key in the URL, does local storage USE those filters, or override them with your local filters?
• If you keep editing filters locally, and the URL updates the filter key accordingly, are all URL and navigation aspects preserved (i.e. does hitting back go to the prior set values), or does this change behavior?
• Are there any situations where the filter state stored locally might be obsolete compared to the data/filters available as fetched from the server in the normal workflow? I.e. might you be applying a filter on a column/value that no longer exists in the DB? Of course the normal native filter key might share this problem if it is indeed a problem.
• Could any of the bug/feature as it exists today be improved by additional push state entries to make navigation more sensible?
This also makes me think of a new feature... just since my brain is on this, I'll mention it... when we apply filters on a dashboard, we get the lovely Apply button that generates a native filters key in the URL. Maybe (if you're one of the dashboard owners, or a use with the permission to do so, which is not always the case) we should have a "Save filters as defaults" button near Apply... that cuold save a lot of time for dashboard authors compared to editing the filters and setting default values one by one.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5781463520
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5946072854
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5797895163
hi @sadpandajoe any idea how to fix this [Translations / babel-extract (pull_request)](https://github.com/apache/superset/actions/runs/35755697013/job/106849602689?pr=43108)
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5802495433
@omsn2 heads up, I rebased this onto `master` so the merge commits are gone. One thing I changed along the way: the last `master` merge had dropped the `useDashboardFilterSync` call in `DashboardPage.tsx` (the MCP filter-apply bit from #44096, unrelated to this PR), so the "unused import" cleanup was papering over a regression. I put the call and import back. Holler if that was intentional.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5808334022
> @omsn2 heads up, I rebased this onto `master` so the merge commits are gone. One thing I changed along the way: the last `master` merge had dropped the `useDashboardFilterSync` call in `DashboardPage.tsx` (the MCP filter-apply bit from #44096, unrelated to this PR), so the "unused import" cleanup was papering over a regression. I put the call and import back. Holler if that was intentional.
Thanks for catching that and doing the rebase, @rusackas!
No, dropping it wasn't intentional at all. It looks like an earlier merge conflict resolution accidentally dropped the actual useDashboardFilterSync() hook call from the component body. Because the call was missing, oxlint started failing CI complaining about an unused import, so the import was just removed to get CI green—papering over the dropped call without realizing it.
Great catch putting them both back! The rebase looks good to me.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5811345087
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3819465236
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
This can overwrite the saved selection with `{}` before the next dashboard has hydrated. `Dashboard` clears `dataMask` on unmount while the old `dashboardInfo.id` remains, so if the dashboard response arrives before charts, this matching-ID effect persists the empty mask and line 272 later restores that empty value. Should persistence wait until this dashboard's restore has completed?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
Review Comment:
This restore only runs when `readyToRender` changes, but the effect depends on `[readyToRender]` alone. During an SPA transition from dashboard A to B where both API results remain ready, it does not rerun for B, so B's saved filters are never loaded. Can the hydration trigger include the dashboard identity (and relevant URL/user inputs)?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
A plain-object check does not establish that this saved mask still matches the dashboard's current filter definition. If an editor keeps a filter ID but retargets it to another column or dataset, the old `extraFormData` is hydrated and can apply the previous selection to the new filter. Should restore validate the stored entries against the current native-filter configuration or invalidate them when it changes?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4144336408
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,543 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__7__1');
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__5__1');
+});
+
+test('skips localStorage restore when ?f= Rison link is present in URL', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['California'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ // Mock getUrlParam to simulate ?f= presence
+ jest.spyOn(require('src/dashboard/util/risonFilters'), 'getRisonFilterParam').mockReturnValue('{}');
+
+ render(
+ <React.Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </React.Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // Verify hydrateDashboard was called without the localStorage filter values
+ // (because ?f= suppresses the fallback)
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ require('src/dashboard/util/risonFilters').getRisonFilterParam.mockRestore();
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage save when historical version preview is active', async () => {
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <React.Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </React.Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: {
+ filters: {
+ 'NATIVE_FILTER-xyz': {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ }
+ }
+ },
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['New Value'] },
+ extraFormData: {},
+ },
+ },
+ user: { userId: 42 },
+ versionHistory: {
+ entityType: 'dashboard',
+ preview: { data: 'snapshot' },
+ },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // Simulate hydration and effect run
+ // Expect no save to localStorage because isVersionPreviewActive is true
+ expect(localStorage.getItem('dashboard__native_filters__42__1')).toBeNull();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>No positive control for save</b></div>
<div id="fix">
The assertion can pass vacuously: no test in this file exercises the save path when `isVersionPreviewActive` is false, so `localStorage.getItem(...)` would also be null if `saveDashboardFilters` never fired (e.g. guard/timing regressions in the `DashboardPage` save effect). Add a positive-control test asserting the key is written with preview inactive, per BITO rule 6262 (assert behavior, not just rendering).
</div>
</div>
<small><i>Code Review Run #f3f68f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,543 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__7__1');
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__5__1');
+});
+
+test('skips localStorage restore when ?f= Rison link is present in URL', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['California'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ // Mock getUrlParam to simulate ?f= presence
+ jest.spyOn(require('src/dashboard/util/risonFilters'), 'getRisonFilterParam').mockReturnValue('{}');
+
+ render(
+ <React.Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </React.Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // Verify hydrateDashboard was called without the localStorage filter values
+ // (because ?f= suppresses the fallback)
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ require('src/dashboard/util/risonFilters').getRisonFilterParam.mockRestore();
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage save when historical version preview is active', async () => {
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <React.Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </React.Suspense>,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>React global without import</b></div>
<div id="fix">
`DashboardPage.test.tsx` imports `{ Suspense }` (line 20) and every pre-existing test uses `<Suspense>`, but this test uses `<React.Suspense>` with no `React` import. Jest passes only because `spec/helpers/setup.ts:38` sets `global.React`; under `tsc` (no `allowUmdGlobalAccess`) the UMD-global value reference is TS2686. Use the existing `Suspense` import for consistency and type safety.
</div>
</div>
<small><i>Code Review Run #f3f68f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3912062553
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
The current code still restores any entry whose ID is retained. Retargeting a filter from `country` to `region` leaves that ID valid, so the old `extraFormData` can continue filtering the new field. Can this PR invalidate saved state when the filter definition changes rather than defer that safety boundary?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3861213837
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
Fixed in the latest commit. The restore path now builds a Set of known filter IDs from dashboard.metadata.native_filter_configuration and filters the stored entries against it before assigning to dataMask. Any entry whose filter ID no longer exists in the current configuration (e.g. a filter that was retargeted to a different column or dataset) is silently dropped, preventing stale extraFormData from being hydrated.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4129007004
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +90,68 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
Review Comment:
This falls back to the unscoped key whenever `userId` is falsy, but the comments on both call sites below explicitly rely on `!= null` (not a truthiness check) so that an authenticated `userId` of `0` isn't treated as anonymous. This function still uses a truthiness check, so a real `userId` of `0` would read/write the guest-shared key instead of its own.
```suggestion
return userId != null
```
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -383,8 +545,58 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ // Also skip when a historical version preview is active: the dashboard
+ // rehydrates with the snapshot's filter defaults, and every other guard
+ // would pass, causing those defaults to overwrite the user's live mask.
+ if (
+ !id ||
+ userId == null ||
+ isVersionPreviewActive ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Opening this dashboard via a permalink, a `native_filters_key`, or a `?f=` Rison link hydrates `dataMask` from that link, and this effect then persists whatever `dataMask` currently holds under the user's own storage key, with no check for whether the value came from an explicit URL rather than an interactive choice. The next time the same user opens the dashboard normally (no link in the URL), they get the shared link's filters instead of their own last selection. Could this effect skip persisting on the same load that just applied an explicit permalink/native-filters-key/Rison restore?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +90,68 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}__${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+ nativeFilters: Record<
+ string,
+ Filter | Divider | ChartCustomization | ChartCustomizationDivider
+ >,
+) {
+ try {
+ const key = getStorageKey(dashboardId, userId);
+
+ // Create a lightweight snapshot of the filter definition (targets and type)
+ // to detect when a dashboard editor retargets or reconfigures a filter,
+ // so we can invalidate the stale extraFormData.
+ const filterDefinitions = Object.fromEntries(
+ Object.entries(nativeFilters).map(([filterId, filter]) => [
+ filterId,
+ {
+ targets: filter.targets,
+ type: filter.filterType,
+ },
+ ]),
+ );
+
+ const nextValue = JSON.stringify({
+ dataMask: nativeFilterMask,
+ filterDefinitions,
+ });
+ // Skip the write if the value has not changed to avoid unnecessary
+ // synchronous main-thread work on every dataMask state update.
+ if (localStorage.getItem(key) !== nextValue) {
+ localStorage.setItem(key, nextValue);
Review Comment:
This writes filter selections into localStorage keyed by the authenticated user, but nothing removes that entry when the user signs out. On a shared machine, the next person to use the browser can still read the previous user's saved filter values (which can include business-sensitive selections like a specific customer or account) from localStorage after logout, even though this app's logout flow already clears its other persisted client-side state for exactly this reason. Should logout also remove keys under this feature's storage prefix?
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,413 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
Review Comment:
None of the new tests exercise the two behaviors this dashboard-filter-persistence feature most recently changed: (1) that the save effect skips persisting while a historical version preview is active, including through an interrupted exit, and (2) that a `?f=` Rison link in the URL suppresses the localStorage restore. A regression on either would currently pass this suite. Could a test be added for each: one that applies a version preview with a different mask and asserts the live saved entry is unchanged after exiting, and one that loads with `?f=` present and asserts the saved entry is not merged into the resulting `dataMask`?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +410,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
Review Comment:
The save effect still isn't limited to native filters: the map it reads from also holds dividers and chart customizations, and it saves any key present there once that key also appears in `dataMask`, so a chart-customization change still gets written into this dashboard's storage entry. It's silently dropped again on restore, since that path checks the dashboard's native-filter configuration instead of this same map, but the write (and its serialization cost) still happens on every such change. Could the save side filter against the native-filter configuration the same way the restore side does, instead of against the full filters map?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +523,47 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ if (
+ !id ||
+ userId == null ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Closing an applied preview clears the preview state (and the flag this effect checks) before the live dashboard data and filter selections are re-hydrated. That leaves a render where the guard is already off but `dataMask`/`nativeFilters` still describe the historical snapshot, and this effect can persist that snapshot under the live key during that window. Could the guard track whether the live state has actually been restored, rather than only whether a preview is currently active, so persistence stays suspended until the exit rehydration completes?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5697245633
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4148280148
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -383,8 +553,63 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ // Also skip when a historical version preview is active: the dashboard
+ // rehydrates with the snapshot's filter defaults, and every other guard
+ // would pass, causing those defaults to overwrite the user's live mask.
+ if (
+ !id ||
+ userId == null ||
+ isVersionPreviewActive ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+
+ if (isRestoringUrlFilters.current) {
Review Comment:
This guard only blocks the very next save-effect run after a URL-driven hydration (permalink, native_filters_key, or legacy Rison `?f=`), then clears itself. Any later, unrelated re-render that changes `nativeFilters` (for example the dashboard-builder container's scope-status dispatch on mount, or the filter bar's auto-apply/default-selection dispatch) re-runs this effect with the ref already false, so it persists the URL-derived mask under the user's own storage key, overwriting whatever selection they actually had saved for this dashboard. Anyone who opens a shared permalink/`?f=` link once and later returns normally gets the link's filters instead of their own. Could the guard track whether the live native-filter mask has actually diverged from the URL-derived one it just hydrated, rather than expiring after exactly one effect run?
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,543 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__7__1');
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__1');
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__42__1');
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ localStorage.removeItem('dashboard__native_filters__5__1');
+});
+
+test('skips localStorage restore when ?f= Rison link is present in URL', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['California'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ // Mock getUrlParam to simulate ?f= presence
+ jest.spyOn(require('src/dashboard/util/risonFilters'), 'getRisonFilterParam').mockReturnValue('{}');
+
+ render(
+ <React.Suspense fallback="loading">
Review Comment:
Both new tests here (and the matching block further down) render with `<React.Suspense>`, but this file only imports the named `Suspense` value and the `ReactNode` type — there's no default `React` import, so `React` is undefined at runtime and these two tests throw before reaching their assertions. That matches the current lint-frontend/pre-commit failures on this PR. Swap both occurrences for the already-imported `<Suspense>`.
Separately, this file's other tests each clean up their own localStorage key with a trailing `removeItem` call instead of a shared teardown, so a thrown assertion in one test can leave its key behind and affect an unrelated later test (one of these two new tests asserts that same key is absent). Worth a shared `afterEach(() => localStorage.clear())`?
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -375,6 +375,14 @@ const RightMenu = ({
try {
window.localStorage.removeItem('redux');
window.sessionStorage.removeItem('login_attempted');
+ // Clear dashboard native filters persistence to prevent leaking saved
+ // filter selections (which may contain business-sensitive data) to the
+ // next user logging in on the same browser profile.
+ Object.keys(window.localStorage).forEach(key => {
Review Comment:
This cleanup only runs through the menu item's onClick handler, but the same element renders a real `<a href>` to the logout URL. Opening that link through a browser context-menu action (for example "open link in new tab") or navigating to it directly skips onClick entirely, so these dashboard-filter keys are never cleared and the next person to use this browser profile can still read the previous user's saved filter selections from localStorage. Could this cleanup instead run from an entry point that fires regardless of how the logout navigation is triggered?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -228,6 +313,93 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else if (getRisonFilterParam()) {
+ // A Rison ?f= filter in the URL encodes an explicit filter intent
+ // (e.g. from a shared link). Loading localStorage state on top would
+ // silently inject the viewer's saved selections (e.g. region=EMEA)
+ // and return narrower data than the URL encodes. Skip the fallback so
+ // the Rison filters below are applied to a clean dataMask.
+ } else if (userId != null) {
+ // Skip localStorage restore for unauthenticated/guest users: they have
+ // no stable identity and reading localStorage here would share filter
+ // state across different guest-token sessions. Use != null (not !!userId)
+ // so a valid userId of 0 is not treated as anonymous.
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ // Reject unversioned entries: the storage key has not shipped, so
+ // there are no legitimate unversioned values in the wild. Falling
+ // back to an ID-only check on an unversioned entry would restore
+ // stale extraFormData for any retargeted filter, so we discard it
+ // outright and let the dashboard open with its configured defaults.
+ const isVersioned =
+ 'dataMask' in savedFilters && 'filterDefinitions' in savedFilters;
+ if (!isVersioned) {
+ // Nothing to restore — proceed with URL/default state
+ } else {
+ const maskToRestore = savedFilters.dataMask;
+ const savedDefinitions = savedFilters.filterDefinitions;
+
+ // Validate that the nested dataMask itself is a non-null plain
+ // object before calling Object.entries. A versioned entry whose
+ // dataMask field is null (e.g. corrupted storage) would otherwise
+ // throw here and prevent the dashboard from hydrating.
+ if (
+ maskToRestore != null &&
+ typeof maskToRestore === 'object' &&
+ !Array.isArray(maskToRestore)
+ ) {
+ // Filter out any null/falsey legacy entries in
+ // native_filter_configuration before looking up IDs, to avoid
+ // dereferencing null when a dashboard has legacy null config rows.
+ const currentFilters = (
+ (dashboard?.metadata?.native_filter_configuration ??
+ []) as Array<NativeFilterConfigEntry | null | undefined>
+ ).filter(
+ (f): f is NativeFilterConfigEntry => f != null && !!f.id,
+ );
+
+ // Only restore entries whose filter ID still exists in the current
+ // native filter configuration. Because this is a versioned entry,
+ // we also verify that the filter's target columns/datasets and type
+ // have not changed. This prevents stale extraFormData from a
+ // retargeted filter from being hydrated.
+ const validatedFilters = Object.fromEntries(
+ Object.entries(maskToRestore).filter(([filterId]) => {
+ const currentConfig = currentFilters.find(
+ f => f.id === filterId,
+ );
+ if (!currentConfig) return false;
+
+ const savedDef = savedDefinitions?.[filterId];
+ if (!savedDef) return false;
+
+ // Validate that the target column(s) and filter type have not changed
+ const currentTargets = JSON.stringify(currentConfig.targets);
+ const savedTargets = JSON.stringify(savedDef.targets);
+
+ if (
Review Comment:
This check verifies the filter's `targets` and `filterType` haven't changed, but not other settings that determine the saved `extraFormData`. If an editor changes a filter's comparator (for example from an exact match to a "contains" match) without changing its target or type, a returning user's restored selection still applies the old comparator's `extraFormData`, producing incorrect query results until they reapply the filter. Should the comparison also cover filter-type-specific settings such as the comparator, not only target and filterType?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3776361513
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +410,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
Review Comment:
Fixed in the latest commit. The save effect now filters the Redux dataMask against the set of configured native filter IDs from state.nativeFilters.filters before persisting. Only entries whose keys match a configured native filter are saved to localStorage. This prevents chart customization state, cross-filter interaction entries, and other transient dataMask entries from being incorrectly serialized and later restored as if they were user-applied filter selections.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
Fixed in the latest commit. The storage key is now scoped to the authenticated user ID using the format superset_dashboard_filters_<userId>_<dashboardId>. This ensures that logging out and signing in as a different user in the same browser profile will not expose or reuse the previous user's filter state. Guest users (who have no userId) gracefully fall back to the dashboard-only key, since guest sessions are inherently ephemeral and not tied to a persistent account.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3996244421
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +289,60 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ const isVersioned =
+ 'dataMask' in savedFilters && 'filterDefinitions' in savedFilters;
+ const maskToRestore = isVersioned
+ ? savedFilters.dataMask
+ : savedFilters;
+ const savedDefinitions = isVersioned
+ ? savedFilters.filterDefinitions
+ : {};
+
+ const currentFilters = (dashboard?.metadata
+ ?.native_filter_configuration ?? []) as NativeFilterConfigEntry[];
+
+ // Only restore entries whose filter ID still exists in the current
+ // native filter configuration. If we have a snapshotted definition
+ // (from the newer versioned schema), we also verify that the filter's
+ // target columns/datasets and type have not changed. This prevents
+ // stale extraFormData from a retargeted filter from being hydrated.
+ const validatedFilters = Object.fromEntries(
+ Object.entries(maskToRestore).filter(([filterId]) => {
+ const currentConfig = currentFilters.find(f => f.id === filterId);
Review Comment:
Dashboards with legacy null entries in `native_filter_configuration` now throw here before `hydrateDashboard`, because `.find` dereferences each entry while the existing hydration path filters falsey values. Could this filter the configuration and cover `[null, validFilter]` before comparing IDs?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
Agreed—embedded users have no `userId`, so this dashboard-only key persists selections across guest-token sessions even though `FilterBar` disables persistence for them. Can save and restore skip users without an authenticated ID?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +267,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
Agreed—the outer-object check still accepts a versioned value whose `dataMask` is null, then `Object.entries(maskToRestore)` throws before the dashboard hydrates. Could the nested schema be validated before iterating it?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
The ref stays true while this page switches dashboards, so if the old Dashboard cleanup clears `dataMask` before the new charts arrive, this effect can still persist an empty mask. Could the guard track the hydrated dashboard ID or skip an empty `nativeFilterMask`?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
The unversioned fallback still restores state by ID alone, so a retargeted filter can apply the old `extraFormData` and then be resaved with the new definition. Since this storage key has not shipped, can unversioned entries be rejected instead?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5852312013
@rusackas all checks looks green please have a look at PR for the merge
Thanks!
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4024212316
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +289,60 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ const isVersioned =
+ 'dataMask' in savedFilters && 'filterDefinitions' in savedFilters;
+ const maskToRestore = isVersioned
+ ? savedFilters.dataMask
+ : savedFilters;
+ const savedDefinitions = isVersioned
+ ? savedFilters.filterDefinitions
+ : {};
+
+ const currentFilters = (dashboard?.metadata
+ ?.native_filter_configuration ?? []) as NativeFilterConfigEntry[];
+
+ // Only restore entries whose filter ID still exists in the current
+ // native filter configuration. If we have a snapshotted definition
+ // (from the newer versioned schema), we also verify that the filter's
+ // target columns/datasets and type have not changed. This prevents
+ // stale extraFormData from a retargeted filter from being hydrated.
+ const validatedFilters = Object.fromEntries(
+ Object.entries(maskToRestore).filter(([filterId]) => {
+ const currentConfig = currentFilters.find(f => f.id === filterId);
Review Comment:
Pre-filter the array with (f): f is NativeFilterConfigEntry => f != null && !!f.id before the find() call.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4173720142
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,593 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
Review Comment:
This test still rejects the saved entry even if the guest guard is removed: the mocked hook returns the configuration in `json_metadata`, but restore reads `dashboard.metadata`, and the saved targets/default mask also differ from the new fixture. Could it return matching `metadata.native_filter_configuration`, targets, and `defaultDataMask` so removing guest exclusion makes the empty-mask assertion fail?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4174011765
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,593 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
Review Comment:
fixed in the latest commit
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4024202448
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
Added if (Object.keys(nativeFilterMask).length === 0) return; after building the mask. This handles the race where the config is non-empty but dataMask has already been cleared during SPA unmount.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4163178948
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -1,3 +1,4 @@
+import { DASHBOARD_FILTERS_STORAGE_PREFIX } from 'src/dashboard/containers/DashboardPage';
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Heavy import bloats menu bundle</b></div>
<div id="fix">
This import pulls `src/dashboard/containers/DashboardPage` (and its static graph: `hydrate` actions, `Dashboard` container, FilterBar `keyValue`, `SyncDashboardState`, `risonFilters`) into the eager menu bundle: `App.tsx:39` and `views/menu.tsx:60` (webpackMode: "eager") statically load `Menu` → `RightMenu`. DashboardPage is deliberately code-split (`webpackChunkName: "Dashboard"`). Move the constant to a leaf module (e.g. `src/utils/localStorageHelpers`) and import from there.
</div>
</div>
<small><i>Code Review Run #486fd2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,593 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+});
+
+test('scopes localStorage key to userId when user is authenticated', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['2024'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'year' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // key is scoped to userId=7 and dashboardId=1
+ localStorage.setItem(
+ 'dashboard__native_filters__7__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID in native_filter_configuration so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'year' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 7 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-xyz': expect.objectContaining({
+ filterState: { value: ['2024'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('does not restore localStorage filters when a nativeFiltersKey is in the URL', async () => {
+ // Put something in localStorage that should be ignored because the URL key takes priority
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify({ 'NATIVE_FILTER-abc': { filterState: { value: ['X'] } } }),
+ );
+
+ const { getFilterValue } = jest.requireMock(
+ 'src/dashboard/components/nativeFilters/FilterBar/keyValue',
+ );
+ (getFilterValue as jest.Mock).mockResolvedValueOnce({
+ 'NATIVE_FILTER-abc': { filterState: { value: ['FromURL'] } },
+ });
+
+ mockGetUrlParam.mockImplementation((param: { name: string }) => {
+ if (param.name === 'native_filters_key') return 'some-key';
+ return null;
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // hydrateDashboard should use the URL-resolved value, not localStorage
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc': expect.objectContaining({
+ filterState: { value: ['FromURL'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('ignores corrupted localStorage data (array) and uses empty dataMask', async () => {
+ // An array is not a valid dataMask shape and must be rejected
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify([1, 2, 3]),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — the corrupted array value must not be used
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+});
+
+test('restores versioned localStorage filters and drops them if targets change', async () => {
+ const savedVersionedData = {
+ dataMask: {
+ 'NATIVE_FILTER-versioned': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-versioned': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+
+ // Use an authenticated user key — restoration skips unauthenticated users.
+ localStorage.setItem(
+ 'dashboard__native_filters__5__1',
+ JSON.stringify(savedVersionedData),
+ );
+
+ // 1. Simulate the dashboard where the target matches
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ const { render } = jest.requireActual('spec/helpers/testing-library');
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should be called with the restored value since target matches
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+
+ unmount();
+ (hydrateDashboard as jest.Mock).mockClear();
+
+ // 2. Simulate the dashboard where the target has changed
+ mockUseDashboard.mockReturnValueOnce({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-versioned',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'country' } }],
+ },
+ ],
+ },
+ },
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 5 },
+ },
+ },
+ );
+
+ // hydrateDashboard should NOT have the dropped filter since target mismatch
+ expect(hydrateDashboard).not.toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-versioned': expect.anything(),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore when ?f= Rison link is present in URL', async () => {
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['California'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-xyz': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ // Mock getRisonFilterParam to simulate ?f= presence
+ jest
+ .spyOn(require('src/dashboard/util/risonFilters'), 'getRisonFilterParam')
+ .mockReturnValue('{}');
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // Verify hydrateDashboard was called without the localStorage filter values
+ // (because ?f= suppresses the fallback)
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({ dataMask: {} }),
+ );
+
+ require('src/dashboard/util/risonFilters').getRisonFilterParam.mockRestore();
+});
+
+test('skips localStorage save when historical version preview is active', async () => {
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: {
+ filters: {
+ 'NATIVE_FILTER-xyz': {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ },
+ },
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['New Value'] },
+ extraFormData: {},
+ },
+ },
+ user: { userId: 42 },
+ versionHistory: {
+ entityType: 'dashboard',
+ preview: { data: 'snapshot' },
+ },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // Simulate hydration and effect run
+ // Expect no save to localStorage because isVersionPreviewActive is true
+ expect(localStorage.getItem('dashboard__native_filters__42__1')).toBeNull();
+});
+
+test('saves to localStorage when historical version preview is inactive', async () => {
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: {
+ filters: {
+ 'NATIVE_FILTER-xyz': {
+ id: 'NATIVE_FILTER-xyz',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ },
+ },
+ dataMask: {
+ 'NATIVE_FILTER-xyz': {
+ filterState: { value: ['New Value'] },
+ extraFormData: {},
+ },
+ },
+ user: { userId: 42 },
+ versionHistory: {
+ entityType: 'dashboard',
+ preview: null
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing trailing comma (oxfmt)</b></div>
<div id="fix">
Line 1209 omits the trailing comma required by the repo formatter (`trailingComma: "all"` in superset-frontend/.oxfmtrc.json, enforced by the `oxfmt-frontend` pre-commit hook that AGENTS.md mandates before every push), so `pre-commit run` fails on this file. Add the comma after `preview: null`, matching the sibling test at line 1149.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/4ddb264/AGENTS.md#L7">AGENTS.md:7</a>
</li>
</ul>
</details>
<small><i>Code Review Run #486fd2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5284331653
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5284017727
@omsn2 please add tests to your PR. Also add a video showing the new functionality you are introducing.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3872849582
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -334,7 +404,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
if (id) getDataMaskApplied();
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [readyToRender]);
+ }, [readyToRender, id]);
Review Comment:
When the dashboard response arrives before charts, this new `id` dependency executes the Rison path while `readyToRender` is false and strips matched `f=` params before hydration. When charts then arrive, the second pass no longer sees those filters, so the dashboard opens unfiltered. Can the URL rewrite happen only once hydration is ready?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
The configuration can still be nonempty while `dataMask` has been cleared, so this persists `{}` before restoration and loses saved selections. Could this wait until restore completes or skip an empty `nativeFilterMask`?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
Keeping the same ID while changing its target still passes this check, so the old `extraFormData` is hydrated into the new filter and can apply the prior column or dataset selection. Can stored state be versioned against the filter definition rather than checked by ID alone?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3861211930
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
Review Comment:
Fixed in the latest commit. Added id to the hydration effect's dependency array (previously [readyToRender] only). During an A→B SPA transition where both API responses are already cached and readyToRender stays true, the effect now re-runs when id changes, ensuring dashboard B's saved filters are correctly loaded.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5801909074
@omsn2 that one's `master` drift, not your branch. The strings it wants (`Security token error` and friends) landed after your last merge, and the template has since caught up on `master`, so merging `master` again should clear babel-extract, and probably the docker-build failures from that same window too.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5594109478
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5424990187
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5421785486
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5284331397
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4024192256
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +267,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
Added an inner guard — maskToRestore != null && typeof maskToRestore === 'object' && !Array.isArray(maskToRestore) — before calling Object.entries(maskToRestore).
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4024186531
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
Restore path: Changed else { ... } branch to else if (userId != null) { ... } — guests hit none of the localStorage code at all.
Save effect: Added userId == null to the early-return guard before any persistence logic.
Uses strict == null (not !userId) so a valid userId of 0 is not treated as anonymous.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5712469570
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4090270909
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `4e951e6`.
Dashboard filter storage keys now include the authenticated user ID alongside the dashboard ID, isolating persisted filter state between users.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5762673492
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3781915389
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+) {
+ try {
+ localStorage.setItem(
+ getStorageKey(dashboardId, userId),
+ JSON.stringify(nativeFilterMask),
+ );
+ } catch {
Review Comment:
Fixed in the latest commit. Added a read-before-write guard: `getSavedDashboardFilters` now reads the existing value and compares it to the new serialized value before calling `setItem`. If the value hasn't changed, the write is skipped entirely, avoiding unnecessary synchronous main-thread work on every `dataMask` update.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +267,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
> Fixed in the latest commit. Added a shape guard before assigning the parsed `localStorage` value to `dataMask`:
> ```ts
> if (savedFilters && typeof savedFilters === 'object' && !Array.isArray(savedFilters))
> ```
> This rejects any corrupted, tampered, or unexpectedly shaped data (arrays, primitives, `null`) before it can reach downstream code that assumes `dataMask` is a plain object map.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
Review Comment:
Fixed in the latest commit. The storage key is now scoped to the authenticated user ID. The actual key format used is dashboard__native_filters__{userId}__{dashboardId} — this ensures that logging out and signing in as a different user in the same browser profile will not expose or reuse the previous user's filter state. Guest users (who have no userId) gracefully fall back to a dashboard-only key (dashboard__native_filters__{dashboardId}), since guest sessions are inherently ephemeral and not tied to a persistent account.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +427,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
Review Comment:
> Fixed in the latest commit. Added 4 unit tests to `DashboardPage.test.tsx` covering:
> - **Load path:** Restores saved filters from `localStorage` when no `permalinkKey` or `nativeFiltersKey` is present in the URL
> - **userId scoping:** Reads from the correct user-scoped key (`dashboard__native_filters__{userId}__{dashboardId}`) when the user is authenticated
> - **URL key priority:** URL-based filter key (`nativeFiltersKey`) takes priority over any stored `localStorage` state
> - **Corrupted data rejection:** An array or primitive stored under the key is ignored and does not get assigned to `dataMask`
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}_${dashboardId}`
Review Comment:
> Fixed — updated the PR description to accurately reflect the implementation. The feature does scope by `userId` when the authenticated user has one (key format: `dashboard__native_filters__{userId}__{dashboardId}`). Guest users (no `userId`) fall back to a dashboard-only key since their sessions are ephemeral. The "No per-user scoping" entry has been removed from the known limitations and replaced with an accurate description.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3888663729
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -334,7 +404,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
if (id) getDataMaskApplied();
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [readyToRender]);
+ }, [readyToRender, id]);
Review Comment:
Great catch. Fixed in the latest commit by adding an early return if (!readyToRender) { return null; } at the top of getDataMaskApplied. This guarantees that the URL rewrite (which strips matched f= params) and Rison injection are fully deferred until both the dashboard and charts are ready.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3959471313
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
I've updated the PR to address your feedback. Instead of just relying on the filter ID, the code now saves a lightweight snapshot of the filter definitions (specifically the targets and filterType) alongside the dataMask in localStorage.
During hydration, if this versioned schema is found, it performs a strict validation: it compares the snapshot against the dashboard's current native_filter_configuration and silently drops any saved state where the target column/dataset or filter type has changed. For backwards compatibility, it falls back to the ID-only check if the older (unversioned) schema is found, so we don't break existing saved states. But going forward, this establishes the exact safety boundary you requested for all newly saved states. Let me know if this looks good!
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4024205643
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
Removed the fallback isVersioned ? savedFilters.dataMask : savedFilters pattern. Non-versioned entries now fall through with a comment and the dashboard opens with its defaults.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4090273421
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +83,65 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}__${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+ nativeFilters: Record<string, any>,
Review Comment:
fixed in latest commit
--
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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4087045615
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `0447be8`.
Storage keys now include the authenticated user ID alongside the dashboard ID, preventing filter state from being shared between identified users in the same browser profile. Guest sessions retain the existing fallback key because they have no user ID.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5804748179
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5967101342
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4173250765
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,593 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
Review Comment:
fixed in the latest commit
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3861208123
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
Fixed in the latest commit. Added an early return in the save effect when nativeFilterIds.length === 0. When Redux clears dataMask on unmount during SPA navigation, the hydratedDashboardId === id guard may still pass momentarily before the next dashboard hydrates. Skipping the write when there are no configured native filter IDs prevents that race from overwriting the user's last valid saved state with {}.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5480335416
> Tagging @kgabryje @michael-s-molina and @villebro on this one, since they might have the most institutional memory around native filter state/storage, and remember reasons we didn't do this before. Seems harmless as long as it's non-breaking for others dpeending on filter state persisted in the key/value store and retrieved via the filter key in the URL. If this is just additive like caching, it's probably a nice addition.
>
> Without having looked at the code yet, I'll note "some does it/should it" questions: • If a dashboard is loaded with no url filter key, and it applies the filters, does/should it ADD the filter key to the URL via push state? One of the benefits of having it there is that you can share that URL with others, so they can benefit from that filter config. Also, just a "truthiness" thing to show more accurately that these filters are applied • If someone sends you a URL with a key in the URL, does local storage USE those filters, or override them with your local filters? • If you keep editing filters locally, and the URL updates the filter key accordingly, are all URL and navigation aspects preserved (i.e. does hitting back go to the prior set values), or does this change behavior? • Are there any situations where the filter state stored locally might be obsolete compared to the data/filters available as fetched from the server in the normal workflow? I.e. might you be applyin
g a filter on a column/value that no longer exists in the DB? Of course the normal native filter key might share this problem if it is indeed a problem. • Could any of the bug/feature as it exists today be improved by additional push state entries to make navigation more sensible?
>
> This also makes me think of a new feature... just since my brain is on this, I'll mention it... when we apply filters on a dashboard, we get the lovely Apply button that generates a native filters key in the URL. Maybe (if you're one of the dashboard owners, or a use with the permission to do so, which is not always the case) we should have a "Save filters as defaults" button near Apply... that cuold save a lot of time for dashboard authors compared to editing the filters and setting default values one by one.
>
> [edit: this might be the longest review comment I've written as a typing-human in MONTHS! Thanks for a thought-provoking PR]
Thanks for the thorough review @rusackas! These are all good questions. I went through the current flow to see how each case behaves today:
**Q1 — If there’s no URL filter key, should restoring filters add one to the URL?**
Yes, this already happens as part of the existing flow.
When `hydrateDashboard` restores the `dataMask`, it updates the Redux state. `FilterBar/index.tsx` then picks up the change through its `useEffect` and calls `publishDataMask`. That eventually calls `createFilterKey` and `history.replace(...)`, which adds the `native_filters_key` to the URL.
So after the filters are restored, the URL reflects the active filter state and can be shared with someone else, just like when filters are applied manually.
One existing limitation is that `publishDataMask` only runs this way for authenticated users. Embedded/guest users won't get the URL key, but that's existing behavior and isn't introduced by this PR.
**Q2 — What happens if both the URL and localStorage have filter state?**
The URL takes precedence.
The restore logic is essentially:
```text
permalinkKey → use permalink state
else nativeFiltersKey → use URL filter state
else → fall back to localStorage
```
So localStorage is only used when there isn't already an explicit filter state in the URL. If someone shares a URL containing a filter key, that state will be used regardless of what filters the recipient has stored locally.
**Q3 — What about Back/Forward navigation when filters change?**
This behavior is unchanged by this PR.
`publishDataMask` currently uses `history.replace(...)`, rather than `history.push(...)`, so changing/applying filters doesn't create separate browser history entries today. The Back button therefore takes you back to the previous page rather than through previous filter states.
I agree that using `push` for explicit user Apply actions could make navigation more intuitive, while keeping `replace` for programmatic changes such as restoration. I think that would be a useful follow-up, but it's outside the scope of this PR.
**Q4 — Could the locally stored state become stale?**
There is some protection against this already. The restore logic checks the stored filter IDs against the current `native_filter_configuration`, so if a filter has been deleted, it won't be restored.
What we don't currently validate is whether the stored `extraFormData` is still valid — for example, if a filter's column or dataset target has changed since the state was stored. In that case, the stale state could potentially result in no/wrong data.
This is also a limitation of the existing server-side `native_filters_key` flow, which stores the `extraFormData` without schema-level invalidation, so I don't think this introduces a new regression. A deeper validation/invalidation mechanism could be handled separately if needed.
**Q5 — Could additional history entries make navigation better?**
Yes, I think so. As mentioned above, using `history.push(...)` for explicit Apply actions could allow users to navigate back through previous filter states.
I'd treat that as a separate improvement to the existing `publishDataMask` behavior rather than something specific to this PR.
**Regarding the "Save filters as defaults" idea:**
I really like this idea. A **"Save as defaults"** action for dashboard owners/users with the appropriate permissions could save the current `dataMask` as the filters' default state, instead of requiring authors to configure each filter's default individually.
It seems like it could fit nicely with the existing `native_filter_configuration` structure. I'd be happy to open a separate issue/feature request for this if that's useful.
Thanks again for the questions — they helped surface some useful follow-up ideas around filter state and browser navigation.
>
Thanks for the thorough review @rusackas! These are all good questions. I went through the current flow to see how each case behaves today:
**Q1 — If there’s no URL filter key, should restoring filters add one to the URL?**
Yes, this already happens as part of the existing flow.
When `hydrateDashboard` restores the `dataMask`, it updates the Redux state. `FilterBar/index.tsx` then picks up the change through its `useEffect` and calls `publishDataMask`. That eventually calls `createFilterKey` and `history.replace(...)`, which adds the `native_filters_key` to the URL.
So after the filters are restored, the URL reflects the active filter state and can be shared with someone else, just like when filters are applied manually.
One existing limitation is that `publishDataMask` only runs this way for authenticated users. Embedded/guest users won't get the URL key, but that's existing behavior and isn't introduced by this PR.
**Q2 — What happens if both the URL and localStorage have filter state?**
The URL takes precedence.
The restore logic is essentially:
```text
permalinkKey → use permalink state
else nativeFiltersKey → use URL filter state
else → fall back to localStorage
```
So localStorage is only used when there isn't already an explicit filter state in the URL. If someone shares a URL containing a filter key, that state will be used regardless of what filters the recipient has stored locally.
**Q3 — What about Back/Forward navigation when filters change?**
This behavior is unchanged by this PR.
`publishDataMask` currently uses `history.replace(...)`, rather than `history.push(...)`, so changing/applying filters doesn't create separate browser history entries today. The Back button therefore takes you back to the previous page rather than through previous filter states.
I agree that using `push` for explicit user Apply actions could make navigation more intuitive, while keeping `replace` for programmatic changes such as restoration. I think that would be a useful follow-up, but it's outside the scope of this PR.
**Q4 — Could the locally stored state become stale?**
There is some protection against this already. The restore logic checks the stored filter IDs against the current `native_filter_configuration`, so if a filter has been deleted, it won't be restored.
What we don't currently validate is whether the stored `extraFormData` is still valid — for example, if a filter's column or dataset target has changed since the state was stored. In that case, the stale state could potentially result in no/wrong data.
This is also a limitation of the existing server-side `native_filters_key` flow, which stores the `extraFormData` without schema-level invalidation, so I don't think this introduces a new regression. A deeper validation/invalidation mechanism could be handled separately if needed.
**Q5 — Could additional history entries make navigation better?**
Yes, I think so. As mentioned above, using `history.push(...)` for explicit Apply actions could allow users to navigate back through previous filter states.
I'd treat that as a separate improvement to the existing `publishDataMask` behavior rather than something specific to this PR.
**Regarding the "Save filters as defaults" idea:**
I really like this idea. A **"Save as defaults"** action for dashboard owners/users with the appropriate permissions could save the current `dataMask` as the filters' default state, instead of requiring authors to configure each filter's default individually.
It seems like it could fit nicely with the existing `native_filter_configuration` structure. I'd be happy to open a separate issue/feature request for this if that's useful.
Thanks again for the questions — they helped surface some useful follow-up ideas around filter state and browser navigation.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5291262020
> @omsn2 please add tests to your PR. Also add a video showing the new functionality you are introducing.
I have updated the PR Description
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5467895965
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4033414039
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +523,47 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ if (
+ !id ||
+ userId == null ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Added isVersionPreviewActive from selectIsDashboardVersionPreviewActive (already used in Header and DashboardBuilder) to the save effect's early-return guard. When a version preview is active, the entire persistence write is skipped. Added it to the useEffect deps array so it re-evaluates as preview opens/closes.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4032779346
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +289,87 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else if (userId != null) {
Review Comment:
This localStorage fallback still runs when the URL contains an `f=` Rison filter, and the later injection overlays only the URL's matching fields onto this restored mask. A shared `?f=(product:Widget)` link can therefore silently retain the viewer's saved `region=EMEA` filter and return narrower data than the URL encodes; should any Rison filter skip the fallback like the other URL filter keys?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +523,47 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ if (
+ !id ||
+ userId == null ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Opening a historical version rehydrates the same dashboard ID with that snapshot's filter defaults, so every guard here passes and this overwrites the user's saved live mask. If they refresh or navigate away before exiting preview, those historical defaults are restored on the next live visit; could persistence be disabled while a version preview is applied?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5910469496
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4061898270
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `c1c1775`.
Dashboard filter storage keys now include the authenticated user ID alongside the dashboard ID, preventing authenticated users from sharing persisted filter state in the same browser profile.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5972807579
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 an incremental AI Review.
- `/review full` - 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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3773907579
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
**Suggestion:** The storage key contains only the dashboard ID, so logging out and signing in as another user in the same browser profile reuses the previous user's filter selections. This can expose private filter values and apply user-specific state to another user's session; include an authenticated user or instance-specific scope in the key, or clear these entries on logout. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Shared-browser users can see another user's filter selections.
- ⚠️ User-specific filters can affect another user's dashboard queries.
- ⚠️ Logout cleanup omits the new dashboard storage namespace.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d7dca4fabe61445a9565c00660e22a4b&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=d7dca4fabe61445a9565c00660e22a4b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 84:84
**Comment:**
*Security: The storage key contains only the dashboard ID, so logging out and signing in as another user in the same browser profile reuses the previous user's filter selections. This can expose private filter values and apply user-specific state to another user's session; include an authenticated user or instance-specific scope in the key, or clear these entries on logout.
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%2F43108&comment_hash=0b277ac9eed2478d8e06d22457e3c3aacfefbf1a10df22602c5e23d884083183&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43108&comment_hash=0b277ac9eed2478d8e06d22457e3c3aacfefbf1a10df22602c5e23d884083183&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +410,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
Review Comment:
**Suggestion:** The effect persists the entire Redux data mask rather than only native filter entries. That mask includes chart customization and other interaction entries, and dashboard hydration explicitly reads persisted entries for configured chart customizations, so revisiting a dashboard can restore stale chart customization state even though this feature is intended to persist filter selections. Persist only native filter IDs (and exclude synthetic or transient entries) before saving. [state/lifecycle]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart customizations can remain stale across dashboard visits.
- ❌ Reopened dashboards may use unintended query controls.
- ⚠️ Synthetic URL-filter entries can outlive their originating URL.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9e108d2066e64e08bfc95ee08979a630&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=9e108d2066e64e08bfc95ee08979a630&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 418:418
**Comment:**
*State Lifecycle: The effect persists the entire Redux data mask rather than only native filter entries. That mask includes chart customization and other interaction entries, and dashboard hydration explicitly reads persisted entries for configured chart customizations, so revisiting a dashboard can restore stale chart customization state even though this feature is intended to persist filter selections. Persist only native filter IDs (and exclude synthetic or transient entries) before saving.
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%2F43108&comment_hash=517e02a47d9375dec8f6f26cffa9474b83d8bb806fb2b9c53bb9a9f9f31dbcc4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43108&comment_hash=517e02a47d9375dec8f6f26cffa9474b83d8bb806fb2b9c53bb9a9f9f31dbcc4&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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5278204621
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | c826857e85930352632ddd31f2444e156815d95e |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a7d868b754e52000823e660 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43108--superset-docs-preview.netlify.app](https://deploy-preview-43108--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLDoAgDADRu3QtkfhB4g08RrE1kKiQ0o0x3l12s3jzQpF8FYUVoIMgeO-x9cGofWGpqaohrDFkFDJHOpXFVEXlxonLmZ-N2uBwIe98WOaJ58Fa64eRnbPw_Q) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43108--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5278186467
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5278452699
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43108?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.11765%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.65%. Comparing base ([`a0d7ec9`](https://app.codecov.io/gh/apache/superset/commit/a0d7ec9fafd7a0c2ea1caac5aeef4759c69f5f2e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c826857`](https://app.codecov.io/gh/apache/superset/commit/c826857e85930352632ddd31f2444e156815d95e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43108?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rontend/src/dashboard/containers/DashboardPage.tsx](https://app.codecov.io/gh/apache/superset/pull/43108?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Fcontainers%2FDashboardPage.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZFBhZ2UudHN4) | 94.11% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43108?src=pr&el=tree&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 #43108 +/- ##
=======================================
Coverage 66.64% 66.65%
=======================================
Files 2863 2863
Lines 162155 162172 +17
Branches 37405 37409 +4
=======================================
+ Hits 108074 108090 +16
- Misses 52022 52023 +1
Partials 2059 2059
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43108/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/43108/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.66% <94.11%> (+<0.01%)` | :arrow_up: |
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/43108?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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3888664382
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
Currently, the check validates that the stored filter ID still actively exists in the dashboard's native_filter_configuration (which effectively catches filter deletions and complete replacements). Adding deep versioning (e.g., by hashing filter targets and storing that alongside the dataMask in localStorage) would require a change to the client-side storage schema for native filters. Since this PR already greatly improves the baseline persistence safety (by scoping to userId and validating IDs), I'd prefer to handle deeper schema-based invalidation in a separate follow-up PR to keep this one focused.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5291450009
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43108:
URL: https://github.com/apache/superset/pull/43108#issuecomment-5594474698
@omsn2 this latest push looks like it actually closes out @sadpandajoe's retargeting concern, snapshotting each filter's targets/type and dropping the saved entry if they've changed since. CI's still chugging through on this commit though. @sadpandajoe mind taking another look at the versioning approach when you get a chance?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3996245660
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
Agreed. Guest/embedded sessions should not persist filter state because there is no stable identity to scope it to, and this matches `FilterBar`’s behavior.
Skip both restoration and persistence when `userId` is unavailable:
```typescript
// Before attempting localStorage restoration
if (userId == null) {
// Continue with URL/default state only
} else {
const savedFilters = getSavedDashboardFilters(id, userId);
// existing validation and restoration logic
}
```
And in the persistence effect:
```typescript
useEffect(() => {
if (
!id ||
userId == null ||
hydratedDashboardId !== id ||
!isDashboardHydrated.current
) {
return;
}
// existing persistence logic
}, [id, hydratedDashboardId, fullDataMask, nativeFilters, userId]);
```
Use `userId == null` rather than a truthiness check so a valid ID of `0` is not treated as anonymous. The existing authenticated-user key format then prevents cross-user reuse, while embedded/guest sessions neither read nor write dashboard filter state. Tests expecting anonymous localStorage restoration should be updated to use an authenticated `userId`; add a test confirming anonymous sessions skip both operations.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4171710813
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +631,593 @@ test('clears undo history after hydrating the dashboard', async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is present', async () => {
+ // Versioned format: { dataMask, filterDefinitions }. The current code rejects
+ // unversioned entries (ID-only check) since the key has not shipped yet.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-abc123': {
+ filterState: { value: ['California'] },
+ extraFormData: {
+ filters: [{ col: 'state', op: 'IN', val: ['California'] }],
+ },
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-abc123': {
+ targets: [{ column: { name: 'state' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Authenticated user — key format: dashboard__native_filters__{userId}__{dashboardId}
+ localStorage.setItem(
+ 'dashboard__native_filters__42__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ // Include the filter ID (with matching targets/type) so versioned validation passes.
+ mockUseDashboard.mockReturnValue({
+ result: {
+ ...mockDashboard,
+ metadata: {
+ native_filter_configuration: [
+ {
+ id: 'NATIVE_FILTER-abc123',
+ filterType: 'filter_select',
+ targets: [{ column: { name: 'state' } }],
+ },
+ ],
+ },
+ },
+ error: null,
+ });
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: 42 },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(hydrateDashboard).toHaveBeenCalledWith(
+ expect.objectContaining({
+ dataMask: expect.objectContaining({
+ 'NATIVE_FILTER-abc123': expect.objectContaining({
+ filterState: { value: ['California'] },
+ }),
+ }),
+ }),
+ );
+});
+
+test('skips localStorage restore for guest/embedded users (userId is undefined)', async () => {
+ // Guest users have no stable identity; reading localStorage could share
+ // filter state across different guest-token sessions, so the restore path
+ // must be skipped entirely when userId is null/undefined.
+ const savedVersioned = {
+ dataMask: {
+ 'NATIVE_FILTER-guest': {
+ filterState: { value: ['SomeValue'] },
+ extraFormData: {},
+ },
+ },
+ filterDefinitions: {
+ 'NATIVE_FILTER-guest': {
+ targets: [{ column: { name: 'col' } }],
+ type: 'filter_select',
+ },
+ },
+ };
+ // Write under the guest (dashboard-only) key — should never be read.
+ localStorage.setItem(
+ 'dashboard__native_filters__1',
+ JSON.stringify(savedVersioned),
+ );
+
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ user: { userId: undefined },
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ // dataMask should be empty — guest users skip localStorage restoration
+ expect(hydrateDashboard).toHaveBeenCalledWith(
Review Comment:
This guest-restore test still passes if the guest exclusion is removed: the default mock dashboard has no native-filter configuration, so restoration discards the saved entry for that reason instead. Could the fixture include the saved filter's matching ID, targets, and type, then assert that guest hydration still receives an empty mask?
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "omsn2 (via GitHub)" <gi...@apache.org>.
omsn2 commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4033412294
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +289,87 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else if (userId != null) {
Review Comment:
Added getRisonFilterParam() as a priority check between nativeFiltersKey and the userId localStorage branch. When a Rison ?f= filter is in the URL, the localStorage fallback is skipped (with an empty-branch comment explaining why), and the Rison filters below are injected onto a clean {} dataMask instead.
--
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] feat(dashboard): persist native filter state to localStorage across navigation [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 #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4074058475
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `85e6dae`.
Dashboard filter storage keys now include the authenticated user ID alongside the dashboard ID, preventing authenticated users from sharing persisted filter state in the same browser profile.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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] feat(dashboard): persist native filter state to localStorage across navigation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4088378994
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +83,65 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}__${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+ nativeFilters: Record<string, any>,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>any type violates standard</b></div>
<div id="fix">
`saveDashboardFilters` takes `nativeFilters: Record<string, any>`, so `filter.targets`/`filter.filterType` are untyped. The repo standard forbids `any` (dev-standard.mdc). Type it with the `Filter | Divider | ChartCustomization | ChartCustomizationDivider` union used by `nativeFilterReducer`.
</div>
</div>
<small><i>Code Review Run #dc174c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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