Skip to content

fix(embed): prevent random crash for filters bar in embedded dashboard #32294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hainenber wants to merge 6 commits into
base: master
Choose a base branch
from
+45 −17
Draft

fix(embed): prevent random crash for filters bar in embedded dashboard #32294

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1701ee
fix(embed): prevent random crash for filters bar in embedded dashboard
hainenber Feb 18, 2025
fb020b6
fix(embed): repair broken bootstrap data before parsing as json
hainenber Feb 18, 2025
6265fff
fix(ci): increase timeout for slow UploadDataModal Jest test
hainenber Feb 18, 2025
cd8bc60
feat(frontend/utils): parse potentially invalid bootstrap data before…
hainenber Feb 21, 2025
fe0ff83
feat(frontend/utils): wrap json-repaired bootstrap data in try-catch …
hainenber Feb 27, 2025
e19fa11
chore(frontend/utils): reduce duplicate logging
hainenber Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions superset-frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions superset-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@
"js-yaml-loader": "^1.2.2",
"json-bigint": "^1.0.0",
"json-stringify-pretty-compact": "^2.0.0",
"jsonrepair": "^3.12.0",
"lodash": "^4.17.21",
"luxon": "^3.5.0",
"mapbox-gl": "^2.10.0",
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/dashboard/components/Header/Header.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const initialState = {
common: {
conf: {
DASHBOARD_AUTO_REFRESH_INTERVALS: [
[0, "Don't refresh"],
[0, 'Do not refresh'],
[10, '10 seconds'],
],
},
Expand Down
24 changes: 12 additions & 12 deletions superset-frontend/src/dashboard/components/RefreshIntervalModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const createProps = () => ({
common: {
conf: {
DASHBOARD_AUTO_REFRESH_INTERVALS: [
[0, "Don't refresh"],
[0, 'Do not refresh'],
[10, '10 seconds'],
[30, '30 seconds'],
[60, '1 minute'],
Expand Down Expand Up @@ -113,7 +113,7 @@ const openRefreshIntervalModal = async () => {

const displayOptions = async () => {
// Click default refresh interval option to display other options
userEvent.click(screen.getByText(/don't refresh/i));
userEvent.click(screen.getByText(/do not refresh/i));
};

const defaultRefreshIntervalModalProps = {
Expand Down Expand Up @@ -148,9 +148,9 @@ test('renders refresh interval options', async () => {
await openRefreshIntervalModal();
await displayOptions();

// Assert that both "Don't refresh" instances exist
// Assert that both "Do not refresh" instances exist
// - There will be two at this point, the default option and the dropdown option
const dontRefreshInstances = screen.getAllByText(/don't refresh/i);
const dontRefreshInstances = screen.getAllByText(/do not refresh/i);
expect(dontRefreshInstances).toHaveLength(2);
dontRefreshInstances.forEach(option => {
expect(option).toBeInTheDocument();
Expand All @@ -177,34 +177,34 @@ test('should change selected value', async () => {
render(setup(editModeOnProps));
await openRefreshIntervalModal();

// Initial selected value should be "Don't refresh"
const selectedValue = screen.getByText(/don't refresh/i);
expect(selectedValue.title).toMatch(/don't refresh/i);
// Initial selected value should be "Do not refresh"
const selectedValue = screen.getByText(/do not refresh/i);
expect(selectedValue.title).toMatch(/do not refresh/i);

// Display options and select "10 seconds"
await displayOptions();
userEvent.click(screen.getByText(/10 seconds/i));

// Selected value should now be "10 seconds"
expect(selectedValue.title).toMatch(/10 seconds/i);
expect(selectedValue.title).not.toMatch(/don't refresh/i);
expect(selectedValue.title).not.toMatch(/do not refresh/i);
});

test('should change selected value to custom value', async () => {
render(setup(editModeOnProps));
await openRefreshIntervalModal();

// Initial selected value should be "Don't refresh"
const selectedValue = screen.getByText(/don't refresh/i);
expect(selectedValue.title).toMatch(/don't refresh/i);
// Initial selected value should be "Do not refresh"
const selectedValue = screen.getByText(/do not refresh/i);
expect(selectedValue.title).toMatch(/do not refresh/i);

// Display options and select "Custom interval"
await displayOptions();
userEvent.click(screen.getByText(/Custom interval/i));

// Selected value should now be "Custom interval"
expect(selectedValue.title).toMatch(/Custom interval/i);
expect(selectedValue.title).not.toMatch(/don't refresh/i);
expect(selectedValue.title).not.toMatch(/do not refresh/i);
});

test('should save a newly-selected value', async () => {
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/features/databases/UploadDataModel/UploadDataModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ test('Columnar, does not render the rows', () => {
});

test('database and schema are correctly populated', async () => {
jest.setTimeout(10000);
jest.setTimeout(20000);
Copy link
Member

@sadpandajoe sadpandajoe Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering why we are bumping the timeout here.

Copy link
Contributor Author

@hainenber hainenber Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did observed that this test suite is failing more often than previously (1, 2, 3).

I'm picking up my own failed CI checks as examples as it's easier to find them in the personal mailbox.

render(<UploadDataModal {...csvProps} />, {
useRedux: true,
});
Expand Down
5 changes: 4 additions & 1 deletion superset-frontend/src/utils/getBootstrapData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
* under the License.
*/

import { jsonrepair } from 'jsonrepair';
import { BootstrapData } from 'src/types/bootstrapTypes';
import { DEFAULT_BOOTSTRAP_DATA } from 'src/constants';

export default function getBootstrapData(): BootstrapData {
const appContainer = document.getElementById('app');
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
return dataBootstrap
? JSON.parse(jsonrepair(dataBootstrap))
: DEFAULT_BOOTSTRAP_DATA;
}
2 changes: 1 addition & 1 deletion superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ class D3TimeFormat(TypedDict, total=False):
DASHBOARD_AUTO_REFRESH_MODE: Literal["fetch", "force"] = "force"
# Dashboard auto refresh intervals
DASHBOARD_AUTO_REFRESH_INTERVALS = [
[0, "Don't refresh"],
[0, "Do not refresh"],
[10, "10 seconds"],
[30, "30 seconds"],
[60, "1 minute"],
Expand Down
Loading