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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
