feat(dashboard): support create dashboard onboarding workflow#37929
feat(dashboard): support create dashboard onboarding workflow#37929reynoldmorel wants to merge 6 commits intoapache:masterfrom
Conversation
reynoldmorel
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
github-actions
bot
added
risk:db-migration
dosubot
bot
added
change:frontend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@reynoldmorel, is this work connected to #37680? Is there a SIP that covers these changes? The current description doesn’t provide enough context for reviewers to fully understand the intent. If you’re still drafting the PR, feel free to mark it as a draft and request a review once everything’s ready. Additionally, if this is indeed related to #37680, have you had a chance to address this comment? |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #c80ea8
Actionable Suggestions - 9
-
superset-frontend/src/embedded/index.tsx - 3
- Double translation call · Line 91-95
- UI Flicker on Load · Line 133-133
- Loading state stuck on error · Line 138-144
-
superset-frontend/src/components/OnboardingWorkflows/index.test.tsx - 1
- Test clicks invalid button · Line 76-76
-
superset-frontend/src/userOnboardingWorkflow/actions.ts - 2
- Loading state not reset · Line 77-91
- Loading state and redundant API call · Line 93-113
-
superset/migrations/versions/2026-02-04_17-02_a55bbb4da9a6_add_create_dashboard_with_none_existing_.py - 1
- Spelling errors in migration text · Line 17-48
-
superset-frontend/src/components/OnboardingWorkflows/index.tsx - 1
- Incomplete callback destructuring and faulty condition · Line 62-66
-
superset-frontend/src/components/OnboardingWorkflows/utils.ts - 1
- Missing error handling for JSON.parse · Line 57-57
Additional Suggestions - 16
-
superset/views/users/api.py - 2
-
Missing API Documentation · Line 223-223The set_onboarding_workflow_visited method lacks an OpenAPI docstring, unlike other methods in the class. This makes the API specification incomplete for consumers. It looks like the method should include documentation similar to get_my_onboarding_workflows.
Code suggestion
@@ -223,2 +223,15 @@ - def set_onboarding_workflow_visited(self, onboarding_workflow_id: int) -> Response: - UserOnboardingWorkflowDAO.set_visited(g.user.id, onboarding_workflow_id) + def set_onboarding_workflow_visited(self, onboarding_workflow_id: int) -> Response: + """Set onboarding workflow as visited + --- + patch: + summary: Set onboarding workflow as visited + description: >- + Sets the specified onboarding workflow as visited for the current user. + parameters: + - name: onboarding_workflow_id + in: path + required: true + schema: + type: integer + responses: + 200: + description: Onboarding workflow set as visited + 401: + $ref: '#/components/responses/401' + """ + UserOnboardingWorkflowDAO.set_visited(g.user.id, onboarding_workflow_id)
-
Inconsistent Response Format · Line 225-225The set_onboarding_workflow_visited method uses Response(status=200) instead of self.response(200), which is inconsistent with other methods in the class like get_me and update_me. While this doesn't affect functionality, it should be consistent for maintainability.
Code suggestion
@@ -224,2 +224,2 @@ - UserOnboardingWorkflowDAO.set_visited(g.user.id, onboarding_workflow_id) - return Response(status=200) + UserOnboardingWorkflowDAO.set_visited(g.user.id, onboarding_workflow_id) + return self.response(200)
-
-
tests/integration_tests/base_tests.py - 2
-
Grammatical error in description · Line 178-178The description string contains a grammatical error: 'none existing chart' should be 'no existing chart' to match standard English usage.
Code suggestion
@@ -175,2 +175,2 @@ - description="Onboarding workflow for creating a " - "dashboard with none existing chart", + description="Onboarding workflow for creating a " + "dashboard with no existing chart",
-
Missing type hint · Line 165-165Per development standards requiring type hints for all new Python code, add the missing return type annotation to improve code clarity and enable MyPy validation.
-
-
superset-frontend/src/components/OnboardingWorkflows/useShouldRenderOnboardingWorkflow.test.tsx - 2
-
Incomplete test coverage for hook logic · Line 44-70The test suite only covers two scenarios but the hook has four conditions that can fail independently. Consider adding tests for: feature flag disabled but workflow exists, workflow exists but shouldVisit=false, unsupported workflow name, and feature flag enabled but no workflow set.
-
Typo in test mock description · Line 32-32The mock description contains a grammatical error: 'none existing' should be 'no existing'. This appears to be a typo in the test data.
Code suggestion
@@ -32,1 +32,1 @@ - 'Onboarding workflow for creating a dashboard with none existing chart', + 'Onboarding workflow for creating a dashboard with no existing chart',
-
-
tests/unit_tests/fixtures/common.py - 1
-
Grammatical error in test fixture description · Line 109-109The description string contains a grammatical error: 'none existing chart' should be 'no existing chart'. This appears to be a typo that could confuse readers of the test data.
Code suggestion
@@ -108,2 +108,2 @@ - description="Onboarding workflow for creating a " - "dashboard with none existing chart", + description="Onboarding workflow for creating a " + "dashboard with no existing chart",
-
-
superset/commands/user_onboarding_workflow/exceptions.py - 1
-
Improve error message formatting for None identifiers · Line 25-35The __init__ method always formats user_id and onboarding_workflow_id into a string for object_id, leading to messages like 'UserOnboardingWorkflow "user_id=None, onboarding_workflow_id=None" not found.' when both are None. It looks like building object_id conditionally would yield cleaner error messages by omitting None parts and using None when no identifiers exist.
Code suggestion
@@ -31,5 +31,11 @@ - super().__init__( - "UserOnboardingWorkflow", - f"user_id={user_id}, onboarding_workflow_id={onboarding_workflow_id}", - exception, - ) + object_id_parts = [] + if user_id is not None: + object_id_parts.append(f"user_id={user_id}") + if onboarding_workflow_id is not None: + object_id_parts.append(f"onboarding_workflow_id={onboarding_workflow_id}") + object_id = ", ".join(object_id_parts) if object_id_parts else None + super().__init__( + "UserOnboardingWorkflow", + object_id, + exception, + )
-
-
superset-frontend/src/components/OnboardingWorkflows/CreateDashboardWithNoExistingChart/index.tsx - 1
-
Missing interval cleanup in useEffect · Line 118-124The `setInterval` for checking DOM elements is not cleaned up if the component unmounts before the target is found, which could cause a memory leak. Add a return function in the useEffect.
-
-
superset/daos/user_onboarding_workflow.py - 4
-
Incorrect type import · Line 21-21The import uses rpds.List for the type hint, but typing.List should be used instead for standard type annotations. rpds.List is intended for persistent data structures, not basic type hints.
Code suggestion
@@ -21,1 +21,1 @@ -from rpds import List +from typing import List
-
Missing class docstring · Line 34-34Per development standards in .cursor/rules/dev-standard.mdc, new classes require docstrings. Add a brief docstring to UserOnboardingWorkflowDAO explaining its role.
-
Missing method docstring · Line 36-36Per development standards, new methods require docstrings. Add a docstring to get_by_user_id explaining its parameters and what it returns.
-
Missing method docstring · Line 45-45Per development standards, new methods require docstrings. Add a docstring to set_visited explaining its parameters and what it does.
-
-
superset-frontend/src/components/OnboardingWorkflows/index.test.tsx - 2
-
Typo in mock description · Line 34-34The mock description has a grammatical error: 'none existing' should be 'no existing'.
-
Typo in test prop name · Line 66-66The test prop name has the same typo as the component; update to 'onboardingWorkflowName'.
-
-
superset-frontend/src/components/OnboardingWorkflows/index.tsx - 1
-
Typo in prop name · Line 27-27The prop name has a typo: 'onboardginWorkflowName' should be 'onboardingWorkflowName'. This affects type safety and readability.
-
Review Details
-
Files reviewed - 50 · Commit Range:
30d3588..fb671de- superset-frontend/packages/superset-ui-core/src/components/DynamicEditableTitle/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/DynamicEditableTitle/types.ts
- superset-frontend/packages/superset-ui-core/src/components/EmptyState/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/EmptyState/types.ts
- superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
- superset-frontend/src/components/OnboardingWorkflows/CreateDashboardWithNoExistingChart/constants.tsx
- superset-frontend/src/components/OnboardingWorkflows/CreateDashboardWithNoExistingChart/index.tsx
- superset-frontend/src/components/OnboardingWorkflows/CreateDashboardWithNoExistingChart/utils.ts
- superset-frontend/src/components/OnboardingWorkflows/constants.ts
- superset-frontend/src/components/OnboardingWorkflows/index.test.tsx
- superset-frontend/src/components/OnboardingWorkflows/index.tsx
- superset-frontend/src/components/OnboardingWorkflows/useShouldRenderOnboardingWorkflow.test.tsx
- superset-frontend/src/components/OnboardingWorkflows/useShouldRenderOnboardingWorkflow.ts
- superset-frontend/src/components/OnboardingWorkflows/utils.ts
- superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
- superset-frontend/src/dashboard/components/DashboardGrid.jsx
- superset-frontend/src/dashboard/components/Header/index.jsx
- superset-frontend/src/embedded/index.tsx
- superset-frontend/src/explore/components/ControlPanelsContainer.tsx
- superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
- superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
- superset-frontend/src/explore/components/RunQueryButton/index.tsx
- superset-frontend/src/explore/components/SaveModal.tsx
- superset-frontend/src/features/home/Menu.tsx
- superset-frontend/src/features/home/SubMenu.tsx
- superset-frontend/src/pages/ChartCreation/index.tsx
- superset-frontend/src/pages/DashboardList/index.tsx
- superset-frontend/src/pages/Home/index.tsx
- superset-frontend/src/types/UserOnboardingWorkflow.ts
- superset-frontend/src/userOnboardingWorkflow/actions.test.ts
- superset-frontend/src/userOnboardingWorkflow/actions.ts
- superset-frontend/src/userOnboardingWorkflow/reducers.ts
- superset-frontend/src/userOnboardingWorkflow/types.ts
- superset-frontend/src/userOnboardingWorkflow/utils.ts
- superset-frontend/src/views/App.tsx
- superset-frontend/src/views/store.ts
- superset/commands/user_onboarding_workflow/__init__.py
- superset/commands/user_onboarding_workflow/exceptions.py
- superset/daos/user_onboarding_workflow.py
- superset/migrations/versions/2026-02-04_16-32_91b151e8dac1_add_onboarding_workflows.py
- superset/migrations/versions/2026-02-04_17-02_a55bbb4da9a6_add_create_dashboard_with_none_existing_.py
- superset/models/__init__.py
- superset/models/onboarding_workflow.py
- superset/models/user_onboarding_workflow.py
- superset/views/users/api.py
- superset/views/users/schemas.py
- tests/integration_tests/base_tests.py
- tests/integration_tests/users/api_tests.py
- tests/unit_tests/dao/user_onboarding_workflow_test.py
- tests/unit_tests/fixtures/common.py
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| addDangerToast( | ||
| t( | ||
| t('Sorry, something went wrong. The changes could not be saved.'), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The error handling in enableEmbedded has a double t() call, which may cause incorrect translation behavior. It should be a single t() wrapping the message string.
Code suggestion
Check the AI-generated fix before applying
| addDangerToast( | |
| t( | |
| t('Sorry, something went wrong. The changes could not be saved.'), | |
| ), | |
| ); | |
| addDangerToast( | |
| t('Sorry, something went wrong. The changes could not be saved.'), | |
| ); |
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| }, [endpoint, addInfoToast, addDangerToast, onHide]); | ||
|
|
||
| useEffect(() => { | ||
| setReady(false); |
There was a problem hiding this comment.
The setReady(false) in the useEffect causes a visible flicker: the component renders the UI first, then switches to the Loading state, then back to the UI. Since the initial ready state is true, this creates an unnecessary re-render. Removing this line eliminates the flicker, though it means no Loading indicator during the fetch. If Loading is desired, consider changing the initial ready state to false instead.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/embedded/index.tsx
+++ b/superset-frontend/src/embedded/index.tsx
@@ -132,7 +132,6 @@ export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
useEffect(() => {
- setReady(false);
makeApi<{}, { result: EmbeddedDashboard }>({
method: 'GET',
endpoint,
})({})
.catch(err => {
if ((err as SupersetApiError).status === 404) {
// 404 just means the dashboard isn't currently embedded
return { result: null };
}
addDangerToast(t('Sorry, something went wrong. Please try again.'));
throw err;
})
.then(({ result }) => {
setReady(true);
setEmbedded(result);
setAllowedDomains(result ? result.allowed_domains.join(', ') : '');
});
}, [dashboardId]);
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| .catch(err => { | ||
| if ((err as SupersetApiError).status === 404) { | ||
| // 404 just means the dashboard isn't currently embedded | ||
| return { result: null }; | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| Switchboard.defineMethod( | ||
| 'setThemeMode', | ||
| (payload: { mode: 'default' | 'dark' | 'system' }) => { | ||
| const { mode } = payload; | ||
| log('Received setThemeMode request:', mode); | ||
|
|
||
| try { | ||
| const themeController = getThemeController(); | ||
| addDangerToast(t('Sorry, something went wrong. Please try again.')); | ||
| throw err; |
There was a problem hiding this comment.
If the GET request fails with a non-404 error, the component stays in loading state forever because setReady(true) is never called. The catch should return a resolved promise to allow the then to execute.
Code suggestion
Check the AI-generated fix before applying
- .catch(err => {
- if ((err as SupersetApiError).status === 404) {
- return { result: null };
- }
- addDangerToast(t('Sorry, something went wrong. Please try again.'));
- throw err;
- })
+ .catch(err => {
+ if ((err as SupersetApiError).status === 404) {
+ return { result: null };
+ }
+ addDangerToast(t('Sorry, something went wrong. Please try again.'));
+ return { result: null };
+ })
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| </Provider>, | ||
| ); | ||
|
|
||
| userEvent.click(screen.getByRole('button', { name: 'Open the dialog' })); |
There was a problem hiding this comment.
The test clicks a button named 'Open the dialog' that doesn't exist in the rendered component. Since the OnboardingWorkflow renders Joyride with run=true and one step, it starts immediately, and the 'Last' button finishes the tour. Remove this incorrect click to fix the test failure.
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| export function fetchCurrentUserOnboardingworkflows() { | ||
| return async function (dispatch: ThunkDispatch<any, undefined, AnyAction>) { | ||
| try { | ||
| dispatch(setUserOnboardingWorkflowsIsLoading(true)); | ||
| const response = await getCurrentUserOnboardingWorkflowsApi()(); | ||
|
|
||
| const mappedUserOnboardingWorkflows: UserOnboardingWorkflow[] = | ||
| mapToUserOnboardingWorkflows(response.result); | ||
|
|
||
| dispatch(setUserOnboardingWorkflows(mappedUserOnboardingWorkflows)); | ||
| } catch (error) { | ||
| dispatch(setUserOnboardingWorkflowsError(error.message)); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The fetchCurrentUserOnboardingworkflows thunk sets loading to true but never resets it to false, causing the UI to remain in a loading state indefinitely. Add a finally block with dispatch(setUserOnboardingWorkflowsIsLoading(false)) to ensure loading is properly managed.
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| export function markCurrentUserOnboardingWorkflowAsVisited( | ||
| userOnboardingWorkflowId: number, | ||
| ) { | ||
| return async function (dispatch: ThunkDispatch<any, undefined, AnyAction>) { | ||
| try { | ||
| dispatch(setUserOnboardingWorkflowsIsLoading(true)); | ||
|
|
||
| await markCurrentUserOnboardingWorkflowAsVisitedApi( | ||
| userOnboardingWorkflowId, | ||
| )(); | ||
|
|
||
| const response = await getCurrentUserOnboardingWorkflowsApi()(); | ||
|
|
||
| const mappedUserOnboardingWorkflows: UserOnboardingWorkflow[] = | ||
| mapToUserOnboardingWorkflows(response.result); | ||
|
|
||
| dispatch(setUserOnboardingWorkflows(mappedUserOnboardingWorkflows)); | ||
| } catch (error) { | ||
| dispatch(setUserOnboardingWorkflowsError(error.message)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Similar to the fetch thunk, markCurrentUserOnboardingWorkflowAsVisited sets loading to true without resetting it. Additionally, it ignores the UserOnboardingWorkflow[] returned by the PATCH API and makes an unnecessary GET request. Use the PATCH response directly to update state and add a finally block for loading.
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| """Add oboarding workflow for creating a dashboard with none existing chart | ||
|
|
||
| Revision ID: a55bbb4da9a6 | ||
| Revises: 91b151e8dac1 | ||
| Create Date: 2026-02-04 17:02:54.527420 | ||
|
|
||
| """ | ||
|
|
||
| import sqlalchemy as sa | ||
| from alembic import op | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "a55bbb4da9a6" | ||
| down_revision = "91b151e8dac1" | ||
|
|
||
| onboarding_workflow_table = sa.Table( | ||
| "onboarding_workflow", | ||
| sa.MetaData(), | ||
| sa.Column("id", sa.Integer, primary_key=True), | ||
| sa.Column("name", sa.String(100), unique=True), | ||
| sa.Column("description", sa.String(255)), | ||
| ) | ||
|
|
||
|
|
||
| def upgrade(): | ||
| bind = op.get_bind() | ||
|
|
||
| # Insert the new onboarding workflow | ||
| insert_stmt = onboarding_workflow_table.insert().values( | ||
| name="CREATE_DASHBOARD_WITH_NO_EXISTING_CHART", | ||
| description="Onboarding workflow for creating a " | ||
| "dashboard with none existing chart", |
There was a problem hiding this comment.
The migration docstring and inserted description contain spelling errors that should be fixed for clarity and professionalism.
Code suggestion
Check the AI-generated fix before applying
-"""Add oboarding workflow for creating a dashboard with none existing chart
+"""Add onboarding workflow for creating a dashboard with no existing chart
@@ -48,1 +48,1 @@
- "dashboard with none existing chart",
+ "dashboard with no existing chart",
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| const { status, action } = data; | ||
|
|
||
| if ( | ||
| (ORIGIN.BUTTON_CLOSE && action === ACTIONS.CLOSE) || | ||
| status === STATUS.FINISHED |
There was a problem hiding this comment.
The Joyride callback destructures only 'status' and 'action', missing 'origin', and uses an incorrect condition. Currently, it evaluates to 'action === ACTIONS.CLOSE || status === STATUS.FINISHED' (since ORIGIN.BUTTON_CLOSE is truthy), but the intent seems to be marking visited only on button close or finish. Without 'origin', it can't distinguish close sources, potentially marking visited on unintended closes (e.g., keyboard or overlay).
Code suggestion
Check the AI-generated fix before applying
| const { status, action } = data; | |
| if ( | |
| (ORIGIN.BUTTON_CLOSE && action === ACTIONS.CLOSE) || | |
| status === STATUS.FINISHED | |
| const { status, action, origin } = data; | |
| if ( | |
| (origin === ORIGIN.BUTTON_CLOSE && action === ACTIONS.CLOSE) || | |
| status === STATUS.FINISHED |
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return undefined; | ||
| } | ||
|
|
||
| const state = JSON.parse(stateStr); |
There was a problem hiding this comment.
JSON.parse operations lack error handling; if localStorage contains corrupted JSON, SyntaxError will crash the app. Add try-catch blocks returning undefined or skipping on parse failure.
Code Review Run #c80ea8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
SUMMARY
Support Create Dashboard onboarding workflow, to guide the user for first time on how to create a dashboard from a none existent dataset.
For more technical information, request access to the technical design document: https://docs.google.com/document/d/1UI6oDaXoxJcaQm3bH9lW3UHdUNDvBVTh/edit#heading=h.53wsnchj5hg
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Feature enabled:
Screen.Recording.2026-02-12.at.12.02.56.PM.mov
TESTING INSTRUCTIONS
Just enable the feature flag
ENABLE_ONBOARDING_WORKFLOWSand login, you should be able to see the onboarding workflow.ADDITIONAL INFORMATION