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: [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]
