bito-code-review[bot] commented on code in PR #37929: URL: https://github.com/apache/superset/pull/37929#discussion_r2798940766
########## superset-frontend/src/components/OnboardingWorkflows/index.test.tsx: ########## @@ -0,0 +1,86 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import UserOnboardingWorkflow from 'src/types/UserOnboardingWorkflow'; +import { OnboardingWorkflowNames } from './constants'; +import { Provider } from 'react-redux'; +import { configureStore } from '@reduxjs/toolkit'; +import userOnboardingWorkflowsReducer from 'src/userOnboardingWorkflow/reducers'; +import { setUserOnboardingWorkflows } from 'src/userOnboardingWorkflow/actions'; +import { render, screen, waitFor } from '@testing-library/react'; +import OnboardingWorkflow from '.'; +import * as userOnboardingWorkflowActions from 'src/userOnboardingWorkflow/actions'; +import userEvent from '@testing-library/user-event'; + +const userOnboardingWorkflowMock: UserOnboardingWorkflow = { + onboardingWorkflow: { + description: + 'Onboarding workflow for creating a dashboard with none existing chart', + id: 1, + name: OnboardingWorkflowNames.CreateDashboardWithNoExistingChart, + }, + onboardingWorkflowId: 1, + userId: 1, + visitedTimes: 0, + shouldVisit: true, +}; + +const originalFeatureFlags = window.featureFlags; +const markCurrentUserOnboardingWorkflowAsVisitedSpy = jest + .spyOn( + userOnboardingWorkflowActions, + 'markCurrentUserOnboardingWorkflowAsVisited', + ) + .mockImplementation(() => () => Promise.resolve()); + +test('OnboardingWorkflow should mark workflow as visited', async () => { + window.featureFlags = { ENABLE_ONBOARDING_WORKFLOWS: true }; + + const mockStore = configureStore({ + reducer: { + userOnboardingWorkflows: userOnboardingWorkflowsReducer, + }, + }); + + mockStore.dispatch(setUserOnboardingWorkflows([userOnboardingWorkflowMock])); + + render( + <Provider store={mockStore}> + <OnboardingWorkflow + onboardginWorkflowName={ + OnboardingWorkflowNames.CreateDashboardWithNoExistingChart + } + run + steps={[{ target: '#target-element', content: 'Step 1' }]} + /> + <div id="target-element">Target Element</div> + </Provider>, + ); + + userEvent.click(screen.getByRole('button', { name: 'Open the dialog' })); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test clicks invalid button</b></div> <div id="fix"> 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. </div> </div> <small><i>Code Review Run #c80ea8</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/migrations/versions/2026-02-04_17-02_a55bbb4da9a6_add_create_dashboard_with_none_existing_.py: ########## @@ -0,0 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""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", Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Spelling errors in migration text</b></div> <div id="fix"> The migration docstring and inserted description contain spelling errors that should be fixed for clarity and professionalism. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` -"""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", ``` </div> </details> </div> <small><i>Code Review Run #c80ea8</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/embedded/index.tsx: ########## @@ -16,308 +16,293 @@ * specific language governing permissions and limitations * under the License. */ -import 'src/public-path'; - -import { lazy, Suspense } from 'react'; -import ReactDOM from 'react-dom'; -import { BrowserRouter as Router, Route } from 'react-router-dom'; +import { useCallback, useEffect, useState } from 'react'; import { t } from '@apache-superset/core'; -import { makeApi } from '@superset-ui/core'; -import { logging } from '@apache-superset/core'; -import { type SupersetThemeConfig, ThemeMode } from '@apache-superset/core/ui'; -import Switchboard from '@superset-ui/switchboard'; -import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; -import setupClient from 'src/setup/setupClient'; -import setupPlugins from 'src/setup/setupPlugins'; -import { useUiConfig } from 'src/components/UiConfigContext'; -import { store, USER_LOADED } from 'src/views/store'; -import { Loading } from '@superset-ui/core/components'; -import { ErrorBoundary } from 'src/components'; -import { addDangerToast } from 'src/components/MessageToasts/actions'; -import ToastContainer from 'src/components/MessageToasts/ToastContainer'; -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; -import setupCodeOverrides from 'src/setup/setupCodeOverrides'; import { - EmbeddedContextProviders, - getThemeController, -} from './EmbeddedContextProviders'; -import { embeddedApi } from './api'; -import { getDataMaskChangeTrigger } from './utils'; - -setupPlugins(); -setupCodeOverrides({ embedded: true }); - -const debugMode = process.env.WEBPACK_MODE === 'development'; -const bootstrapData = getBootstrapData(); - -function log(...info: unknown[]) { - if (debugMode) logging.debug(`[superset]`, ...info); -} - -const LazyDashboardPage = lazy( - () => - import( - /* webpackChunkName: "DashboardPage" */ 'src/dashboard/containers/DashboardPage' - ), -); - -const EmbededLazyDashboardPage = () => { - const uiConfig = useUiConfig(); - - // Emit data mask changes to the parent window - if (uiConfig?.emitDataMasks) { - log('setting up Switchboard event emitter'); - - let previousDataMask = store.getState().dataMask; - - store.subscribe(() => { - const currentState = store.getState(); - const currentDataMask = currentState.dataMask; - - // Only emit if the dataMask has changed - if (previousDataMask !== currentDataMask) { - Switchboard.emit('observeDataMask', { - ...currentDataMask, - ...getDataMaskChangeTrigger(currentDataMask, previousDataMask), - }); - previousDataMask = currentDataMask; - } - }); - } - - return <LazyDashboardPage idOrSlug={bootstrapData.embedded!.dashboard_id} />; + makeApi, + SupersetApiError, + getExtensionsRegistry, +} from '@superset-ui/core'; +import { styled, css, Alert } from '@apache-superset/core/ui'; +import { + Button, + FormItem, + InfoTooltip, + Input, + Modal, + Loading, + Form, + Space, +} from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { EmbeddedDashboard } from 'src/dashboard/types'; +import { Typography } from '@superset-ui/core/components/Typography'; +import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; + +const extensionsRegistry = getExtensionsRegistry(); + +type Props = { + dashboardId: string; + show: boolean; + onHide: () => void; }; -const EmbeddedRoute = () => ( - <EmbeddedContextProviders> - <Suspense fallback={<Loading />}> - <ErrorBoundary> - <EmbededLazyDashboardPage /> - </ErrorBoundary> - <ToastContainer position="top" /> - </Suspense> - </EmbeddedContextProviders> -); - -const EmbeddedApp = () => ( - <Router basename={applicationRoot()}> - {/* todo (embedded) remove this line after uuids are deployed */} - <Route path="/dashboard/:idOrSlug/embedded/" component={EmbeddedRoute} /> - <Route path="/embedded/:uuid/" component={EmbeddedRoute} /> - </Router> -); - -const appMountPoint = document.getElementById('app')!; - -const MESSAGE_TYPE = '__embedded_comms__'; - -function showFailureMessage(message: string) { - appMountPoint.innerHTML = message; -} - -if (!window.parent || window.parent === window) { - showFailureMessage( - t( - 'This page is intended to be embedded in an iframe, but it looks like that is not the case.', - ), - ); -} - -// if the page is embedded in an origin that hasn't -// been authorized by the curator, we forbid access entirely. -// todo: check the referrer on the route serving this page instead -// const ALLOW_ORIGINS = ['http://127.0.0.1:9001', 'http://localhost:9001']; -// const parentOrigin = new URL(document.referrer).origin; -// if (!ALLOW_ORIGINS.includes(parentOrigin)) { -// throw new Error( -// `[superset] iframe parent ${parentOrigin} is not in the list of allowed origins`, -// ); -// } - -let displayedUnauthorizedToast = false; - -/** - * If there is a problem with the guest token, we will start getting - * 401 errors from the api and SupersetClient will call this function. - */ -function guestUnauthorizedHandler() { - if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401 - displayedUnauthorizedToast = true; - // If a guest user were sent to a login screen on 401, they would have no valid login to use. - // For embedded it makes more sense to just display a message - // and let them continue accessing the page, to whatever extent they can. - store.dispatch( - addDangerToast( - t( - 'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.', - ), - { - duration: -1, // stay open until manually closed - noDuplicate: true, - }, - ), - ); -} - -function start() { - const getMeWithRole = makeApi<void, { result: UserWithPermissionsAndRoles }>({ - method: 'GET', - endpoint: '/api/v1/me/roles/', - }); - return getMeWithRole().then( - ({ result }) => { - // fill in some missing bootstrap data - // (because at pageload, we don't have any auth yet) - // this allows the frontend's permissions checks to work. - bootstrapData.user = result; - store.dispatch({ - type: USER_LOADED, - user: result, +type EmbeddedApiPayload = { allowed_domains: string[] }; + +const stringToList = (stringyList: string): string[] => + stringyList.split(/(?:\s|,)+/).filter(x => x); + +const ButtonRow = styled.div` + display: flex; + flex-direction: row; + justify-content: flex-end; +`; + +export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => { + const { addInfoToast, addDangerToast } = useToasts(); + const [ready, setReady] = useState(true); // whether we have initialized yet + const [loading, setLoading] = useState(false); // whether we are currently doing an async thing + const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); // the embedded dashboard config + const [allowedDomains, setAllowedDomains] = useState<string>(''); + const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false); + + const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`; + // whether saveable changes have been made to the config + const isDirty = + !embedded || + stringToList(allowedDomains).join() !== embedded.allowed_domains.join(); + + const enableEmbedded = useCallback(() => { + setLoading(true); + makeApi<EmbeddedApiPayload, { result: EmbeddedDashboard }>({ + method: 'POST', + endpoint, + })({ + allowed_domains: stringToList(allowedDomains), + }) + .then( + ({ result }) => { + setEmbedded(result); + setAllowedDomains(result.allowed_domains.join(', ')); + addInfoToast(t('Changes saved.')); + }, + err => { + console.error(err); + addDangerToast( + t( + t('Sorry, something went wrong. The changes could not be saved.'), + ), + ); + }, + ) + .finally(() => { + setLoading(false); }); - ReactDOM.render(<EmbeddedApp />, appMountPoint); - }, - err => { - // something is most likely wrong with the guest token - logging.error(err); - showFailureMessage( - t( - 'Something went wrong with embedded authentication. Check the dev console for details.', - ), - ); - }, - ); -} - -/** - * Configures SupersetClient with the correct settings for the embedded dashboard page. - */ -function setupGuestClient(guestToken: string) { - setupClient({ - appRoot: applicationRoot(), - guestToken, - guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME, - unauthorizedHandler: guestUnauthorizedHandler, - }); -} - -function validateMessageEvent(event: MessageEvent) { - // if (!ALLOW_ORIGINS.includes(event.origin)) { - // throw new Error('Message origin is not in the allowed list'); - // } - - if (typeof event.data !== 'object' || event.data.type !== MESSAGE_TYPE) { - throw new Error(`Message type does not match type used for embedded comms`); - } -} - -window.addEventListener('message', function embeddedPageInitializer(event) { - try { - validateMessageEvent(event); - } catch (err) { - log('ignoring message unrelated to embedded comms', err, event); - return; - } - - const port = event.ports?.[0]; - if (event.data.handshake === 'port transfer' && port) { - log('message port received', event); - - Switchboard.init({ - port, - name: 'superset', - debug: debugMode, - }); - - let started = false; - - Switchboard.defineMethod( - 'guestToken', - ({ guestToken }: { guestToken: string }) => { - setupGuestClient(guestToken); - if (!started) { - start(); - started = true; - } - }, - ); - - Switchboard.defineMethod('getScrollSize', embeddedApi.getScrollSize); - Switchboard.defineMethod( - 'getDashboardPermalink', - embeddedApi.getDashboardPermalink, - ); - Switchboard.defineMethod('getActiveTabs', embeddedApi.getActiveTabs); - Switchboard.defineMethod('getDataMask', embeddedApi.getDataMask); - Switchboard.defineMethod('getChartStates', embeddedApi.getChartStates); - Switchboard.defineMethod( - 'getChartDataPayloads', - embeddedApi.getChartDataPayloads, - ); - Switchboard.defineMethod( - 'setThemeConfig', - (payload: { themeConfig: SupersetThemeConfig }) => { - const { themeConfig } = payload; - log('Received setThemeConfig request:', themeConfig); - - try { - const themeController = getThemeController(); - themeController.setThemeConfig(themeConfig); - return { success: true, message: 'Theme applied' }; - } catch (error) { - logging.error('Failed to apply theme config:', error); - throw new Error(`Failed to apply theme config: ${error.message}`); + }, [endpoint, allowedDomains]); + + const disableEmbedded = useCallback(() => { + setShowDeactivateConfirm(true); + }, []); + + const confirmDeactivate = useCallback(() => { + setLoading(true); + makeApi<{}>({ method: 'DELETE', endpoint })({}) + .then( + () => { + setEmbedded(null); + setAllowedDomains(''); + setShowDeactivateConfirm(false); + addInfoToast(t('Embedding deactivated.')); + onHide(); + }, + err => { + console.error(err); + addDangerToast( + t( + 'Sorry, something went wrong. Embedding could not be deactivated.', + ), + ); + }, + ) + .finally(() => { + setLoading(false); + }); + }, [endpoint, addInfoToast, addDangerToast, onHide]); + + 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 }; } - }, - ); - - 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; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Loading state stuck on error</b></div> <div id="fix"> 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. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - .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 }; + }) ``` </div> </details> </div> <small><i>Code Review Run #c80ea8</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/userOnboardingWorkflow/actions.ts: ########## @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { makeApi } from '@superset-ui/core'; +import { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import UserOnboardingWorkflow, { + RawUserOnboardingWorkflow, +} from 'src/types/UserOnboardingWorkflow'; +import mapToUserOnboardingWorkflows from './utils'; + +const getCurrentUserOnboardingWorkflowsApi = () => + makeApi<void, { result: RawUserOnboardingWorkflow[] }>({ + method: 'GET', + endpoint: '/api/v1/me/onboarding-workflows/', + }); + +const markCurrentUserOnboardingWorkflowAsVisitedApi = ( + userOnboardingWorkflowId: number, +) => + makeApi<void, { result: UserOnboardingWorkflow[] }>({ + method: 'PATCH', + endpoint: `/api/v1/me/onboarding-workflows/${userOnboardingWorkflowId}/set-visited`, + }); + +export function setUserOnboardingWorkflows( + userOnboardingWorkflows: UserOnboardingWorkflow[], +) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS', + payload: userOnboardingWorkflows, + }; +} + +export function setUserOnboardingWorkflowsIsLoading(isLoading: boolean) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_IS_LOADING', + payload: isLoading, + }; +} + +export function resetUserOnboardingWorkflowsState() { + return { + type: 'RESET_USER_ONBOARDING_WORKFLOWS_STATE', + }; +} +export function setUserOnboardingWorkflowsError(error: string) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_ERROR', + payload: error, + }; +} + +export function setUserOnboardingWorkflowsStepIndex(stepIndex: number) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_STEP_INDEX', + payload: stepIndex, + }; +} + +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)); + } + }; +} + +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)); + } + }; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Loading state and redundant API call</b></div> <div id="fix"> 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. </div> </div> <small><i>Code Review Run #c80ea8</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/userOnboardingWorkflow/actions.ts: ########## @@ -0,0 +1,135 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { makeApi } from '@superset-ui/core'; +import { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import UserOnboardingWorkflow, { + RawUserOnboardingWorkflow, +} from 'src/types/UserOnboardingWorkflow'; +import mapToUserOnboardingWorkflows from './utils'; + +const getCurrentUserOnboardingWorkflowsApi = () => + makeApi<void, { result: RawUserOnboardingWorkflow[] }>({ + method: 'GET', + endpoint: '/api/v1/me/onboarding-workflows/', + }); + +const markCurrentUserOnboardingWorkflowAsVisitedApi = ( + userOnboardingWorkflowId: number, +) => + makeApi<void, { result: UserOnboardingWorkflow[] }>({ + method: 'PATCH', + endpoint: `/api/v1/me/onboarding-workflows/${userOnboardingWorkflowId}/set-visited`, + }); + +export function setUserOnboardingWorkflows( + userOnboardingWorkflows: UserOnboardingWorkflow[], +) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS', + payload: userOnboardingWorkflows, + }; +} + +export function setUserOnboardingWorkflowsIsLoading(isLoading: boolean) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_IS_LOADING', + payload: isLoading, + }; +} + +export function resetUserOnboardingWorkflowsState() { + return { + type: 'RESET_USER_ONBOARDING_WORKFLOWS_STATE', + }; +} +export function setUserOnboardingWorkflowsError(error: string) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_ERROR', + payload: error, + }; +} + +export function setUserOnboardingWorkflowsStepIndex(stepIndex: number) { + return { + type: 'SET_USER_ONBOARDING_WORKFLOWS_STEP_INDEX', + payload: stepIndex, + }; +} + +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)); + } + }; +} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Loading state not reset</b></div> <div id="fix"> 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. </div> </div> <small><i>Code Review Run #c80ea8</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/embedded/index.tsx: ########## @@ -16,308 +16,293 @@ * specific language governing permissions and limitations * under the License. */ -import 'src/public-path'; - -import { lazy, Suspense } from 'react'; -import ReactDOM from 'react-dom'; -import { BrowserRouter as Router, Route } from 'react-router-dom'; +import { useCallback, useEffect, useState } from 'react'; import { t } from '@apache-superset/core'; -import { makeApi } from '@superset-ui/core'; -import { logging } from '@apache-superset/core'; -import { type SupersetThemeConfig, ThemeMode } from '@apache-superset/core/ui'; -import Switchboard from '@superset-ui/switchboard'; -import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; -import setupClient from 'src/setup/setupClient'; -import setupPlugins from 'src/setup/setupPlugins'; -import { useUiConfig } from 'src/components/UiConfigContext'; -import { store, USER_LOADED } from 'src/views/store'; -import { Loading } from '@superset-ui/core/components'; -import { ErrorBoundary } from 'src/components'; -import { addDangerToast } from 'src/components/MessageToasts/actions'; -import ToastContainer from 'src/components/MessageToasts/ToastContainer'; -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; -import setupCodeOverrides from 'src/setup/setupCodeOverrides'; import { - EmbeddedContextProviders, - getThemeController, -} from './EmbeddedContextProviders'; -import { embeddedApi } from './api'; -import { getDataMaskChangeTrigger } from './utils'; - -setupPlugins(); -setupCodeOverrides({ embedded: true }); - -const debugMode = process.env.WEBPACK_MODE === 'development'; -const bootstrapData = getBootstrapData(); - -function log(...info: unknown[]) { - if (debugMode) logging.debug(`[superset]`, ...info); -} - -const LazyDashboardPage = lazy( - () => - import( - /* webpackChunkName: "DashboardPage" */ 'src/dashboard/containers/DashboardPage' - ), -); - -const EmbededLazyDashboardPage = () => { - const uiConfig = useUiConfig(); - - // Emit data mask changes to the parent window - if (uiConfig?.emitDataMasks) { - log('setting up Switchboard event emitter'); - - let previousDataMask = store.getState().dataMask; - - store.subscribe(() => { - const currentState = store.getState(); - const currentDataMask = currentState.dataMask; - - // Only emit if the dataMask has changed - if (previousDataMask !== currentDataMask) { - Switchboard.emit('observeDataMask', { - ...currentDataMask, - ...getDataMaskChangeTrigger(currentDataMask, previousDataMask), - }); - previousDataMask = currentDataMask; - } - }); - } - - return <LazyDashboardPage idOrSlug={bootstrapData.embedded!.dashboard_id} />; + makeApi, + SupersetApiError, + getExtensionsRegistry, +} from '@superset-ui/core'; +import { styled, css, Alert } from '@apache-superset/core/ui'; +import { + Button, + FormItem, + InfoTooltip, + Input, + Modal, + Loading, + Form, + Space, +} from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { EmbeddedDashboard } from 'src/dashboard/types'; +import { Typography } from '@superset-ui/core/components/Typography'; +import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; + +const extensionsRegistry = getExtensionsRegistry(); + +type Props = { + dashboardId: string; + show: boolean; + onHide: () => void; }; -const EmbeddedRoute = () => ( - <EmbeddedContextProviders> - <Suspense fallback={<Loading />}> - <ErrorBoundary> - <EmbededLazyDashboardPage /> - </ErrorBoundary> - <ToastContainer position="top" /> - </Suspense> - </EmbeddedContextProviders> -); - -const EmbeddedApp = () => ( - <Router basename={applicationRoot()}> - {/* todo (embedded) remove this line after uuids are deployed */} - <Route path="/dashboard/:idOrSlug/embedded/" component={EmbeddedRoute} /> - <Route path="/embedded/:uuid/" component={EmbeddedRoute} /> - </Router> -); - -const appMountPoint = document.getElementById('app')!; - -const MESSAGE_TYPE = '__embedded_comms__'; - -function showFailureMessage(message: string) { - appMountPoint.innerHTML = message; -} - -if (!window.parent || window.parent === window) { - showFailureMessage( - t( - 'This page is intended to be embedded in an iframe, but it looks like that is not the case.', - ), - ); -} - -// if the page is embedded in an origin that hasn't -// been authorized by the curator, we forbid access entirely. -// todo: check the referrer on the route serving this page instead -// const ALLOW_ORIGINS = ['http://127.0.0.1:9001', 'http://localhost:9001']; -// const parentOrigin = new URL(document.referrer).origin; -// if (!ALLOW_ORIGINS.includes(parentOrigin)) { -// throw new Error( -// `[superset] iframe parent ${parentOrigin} is not in the list of allowed origins`, -// ); -// } - -let displayedUnauthorizedToast = false; - -/** - * If there is a problem with the guest token, we will start getting - * 401 errors from the api and SupersetClient will call this function. - */ -function guestUnauthorizedHandler() { - if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401 - displayedUnauthorizedToast = true; - // If a guest user were sent to a login screen on 401, they would have no valid login to use. - // For embedded it makes more sense to just display a message - // and let them continue accessing the page, to whatever extent they can. - store.dispatch( - addDangerToast( - t( - 'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.', - ), - { - duration: -1, // stay open until manually closed - noDuplicate: true, - }, - ), - ); -} - -function start() { - const getMeWithRole = makeApi<void, { result: UserWithPermissionsAndRoles }>({ - method: 'GET', - endpoint: '/api/v1/me/roles/', - }); - return getMeWithRole().then( - ({ result }) => { - // fill in some missing bootstrap data - // (because at pageload, we don't have any auth yet) - // this allows the frontend's permissions checks to work. - bootstrapData.user = result; - store.dispatch({ - type: USER_LOADED, - user: result, +type EmbeddedApiPayload = { allowed_domains: string[] }; + +const stringToList = (stringyList: string): string[] => + stringyList.split(/(?:\s|,)+/).filter(x => x); + +const ButtonRow = styled.div` + display: flex; + flex-direction: row; + justify-content: flex-end; +`; + +export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => { + const { addInfoToast, addDangerToast } = useToasts(); + const [ready, setReady] = useState(true); // whether we have initialized yet + const [loading, setLoading] = useState(false); // whether we are currently doing an async thing + const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); // the embedded dashboard config + const [allowedDomains, setAllowedDomains] = useState<string>(''); + const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false); + + const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`; + // whether saveable changes have been made to the config + const isDirty = + !embedded || + stringToList(allowedDomains).join() !== embedded.allowed_domains.join(); + + const enableEmbedded = useCallback(() => { + setLoading(true); + makeApi<EmbeddedApiPayload, { result: EmbeddedDashboard }>({ + method: 'POST', + endpoint, + })({ + allowed_domains: stringToList(allowedDomains), + }) + .then( + ({ result }) => { + setEmbedded(result); + setAllowedDomains(result.allowed_domains.join(', ')); + addInfoToast(t('Changes saved.')); + }, + err => { + console.error(err); + addDangerToast( + t( + t('Sorry, something went wrong. The changes could not be saved.'), + ), + ); + }, + ) + .finally(() => { + setLoading(false); }); - ReactDOM.render(<EmbeddedApp />, appMountPoint); - }, - err => { - // something is most likely wrong with the guest token - logging.error(err); - showFailureMessage( - t( - 'Something went wrong with embedded authentication. Check the dev console for details.', - ), - ); - }, - ); -} - -/** - * Configures SupersetClient with the correct settings for the embedded dashboard page. - */ -function setupGuestClient(guestToken: string) { - setupClient({ - appRoot: applicationRoot(), - guestToken, - guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME, - unauthorizedHandler: guestUnauthorizedHandler, - }); -} - -function validateMessageEvent(event: MessageEvent) { - // if (!ALLOW_ORIGINS.includes(event.origin)) { - // throw new Error('Message origin is not in the allowed list'); - // } - - if (typeof event.data !== 'object' || event.data.type !== MESSAGE_TYPE) { - throw new Error(`Message type does not match type used for embedded comms`); - } -} - -window.addEventListener('message', function embeddedPageInitializer(event) { - try { - validateMessageEvent(event); - } catch (err) { - log('ignoring message unrelated to embedded comms', err, event); - return; - } - - const port = event.ports?.[0]; - if (event.data.handshake === 'port transfer' && port) { - log('message port received', event); - - Switchboard.init({ - port, - name: 'superset', - debug: debugMode, - }); - - let started = false; - - Switchboard.defineMethod( - 'guestToken', - ({ guestToken }: { guestToken: string }) => { - setupGuestClient(guestToken); - if (!started) { - start(); - started = true; - } - }, - ); - - Switchboard.defineMethod('getScrollSize', embeddedApi.getScrollSize); - Switchboard.defineMethod( - 'getDashboardPermalink', - embeddedApi.getDashboardPermalink, - ); - Switchboard.defineMethod('getActiveTabs', embeddedApi.getActiveTabs); - Switchboard.defineMethod('getDataMask', embeddedApi.getDataMask); - Switchboard.defineMethod('getChartStates', embeddedApi.getChartStates); - Switchboard.defineMethod( - 'getChartDataPayloads', - embeddedApi.getChartDataPayloads, - ); - Switchboard.defineMethod( - 'setThemeConfig', - (payload: { themeConfig: SupersetThemeConfig }) => { - const { themeConfig } = payload; - log('Received setThemeConfig request:', themeConfig); - - try { - const themeController = getThemeController(); - themeController.setThemeConfig(themeConfig); - return { success: true, message: 'Theme applied' }; - } catch (error) { - logging.error('Failed to apply theme config:', error); - throw new Error(`Failed to apply theme config: ${error.message}`); + }, [endpoint, allowedDomains]); + + const disableEmbedded = useCallback(() => { + setShowDeactivateConfirm(true); + }, []); + + const confirmDeactivate = useCallback(() => { + setLoading(true); + makeApi<{}>({ method: 'DELETE', endpoint })({}) + .then( + () => { + setEmbedded(null); + setAllowedDomains(''); + setShowDeactivateConfirm(false); + addInfoToast(t('Embedding deactivated.')); + onHide(); + }, + err => { + console.error(err); + addDangerToast( + t( + 'Sorry, something went wrong. Embedding could not be deactivated.', + ), + ); + }, + ) + .finally(() => { + setLoading(false); + }); + }, [endpoint, addInfoToast, addDangerToast, onHide]); + + useEffect(() => { + setReady(false); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>UI Flicker on Load</b></div> <div id="fix"> 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. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- 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]); ``` </div> </details> </div> <small><i>Code Review Run #c80ea8</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/components/OnboardingWorkflows/utils.ts: ########## @@ -0,0 +1,102 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { CallBackProps, Step } from 'react-joyride'; + +const ONBOARDING_WORKFLOW_LOCAL_STORAGE_KEY = 'onboarding_workflows_state'; + +export default function buildOnboardingWorkflowStepId( + workflowName: string, + stepNumber: number, +) { + return `${workflowName}-step-${stepNumber}`; +} + +export const setZIndexForStepTarget = ( + steps: (Step & { targetClassName: string })[], + stepIdx: number, + zIndex: string, +) => { + if ( + stepIdx > 0 && + stepIdx < steps.length && + document.querySelector(steps[stepIdx].targetClassName) + ) { + const el = document.querySelector( + steps[stepIdx].targetClassName, + ) as HTMLElement; + el.style.zIndex = zIndex; + } +}; + +export const getOnboardingWorkflowStateFromLocalStorage = ( + onboardingWorkflowName: string, +): CallBackProps | undefined => { + const stateStr = localStorage.getItem(ONBOARDING_WORKFLOW_LOCAL_STORAGE_KEY); + + if (!stateStr) { + return undefined; + } + + const state = JSON.parse(stateStr); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing error handling for JSON.parse</b></div> <div id="fix"> 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. </div> </div> <small><i>Code Review Run #c80ea8</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/embedded/index.tsx: ########## @@ -16,308 +16,293 @@ * specific language governing permissions and limitations * under the License. */ -import 'src/public-path'; - -import { lazy, Suspense } from 'react'; -import ReactDOM from 'react-dom'; -import { BrowserRouter as Router, Route } from 'react-router-dom'; +import { useCallback, useEffect, useState } from 'react'; import { t } from '@apache-superset/core'; -import { makeApi } from '@superset-ui/core'; -import { logging } from '@apache-superset/core'; -import { type SupersetThemeConfig, ThemeMode } from '@apache-superset/core/ui'; -import Switchboard from '@superset-ui/switchboard'; -import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; -import setupClient from 'src/setup/setupClient'; -import setupPlugins from 'src/setup/setupPlugins'; -import { useUiConfig } from 'src/components/UiConfigContext'; -import { store, USER_LOADED } from 'src/views/store'; -import { Loading } from '@superset-ui/core/components'; -import { ErrorBoundary } from 'src/components'; -import { addDangerToast } from 'src/components/MessageToasts/actions'; -import ToastContainer from 'src/components/MessageToasts/ToastContainer'; -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; -import setupCodeOverrides from 'src/setup/setupCodeOverrides'; import { - EmbeddedContextProviders, - getThemeController, -} from './EmbeddedContextProviders'; -import { embeddedApi } from './api'; -import { getDataMaskChangeTrigger } from './utils'; - -setupPlugins(); -setupCodeOverrides({ embedded: true }); - -const debugMode = process.env.WEBPACK_MODE === 'development'; -const bootstrapData = getBootstrapData(); - -function log(...info: unknown[]) { - if (debugMode) logging.debug(`[superset]`, ...info); -} - -const LazyDashboardPage = lazy( - () => - import( - /* webpackChunkName: "DashboardPage" */ 'src/dashboard/containers/DashboardPage' - ), -); - -const EmbededLazyDashboardPage = () => { - const uiConfig = useUiConfig(); - - // Emit data mask changes to the parent window - if (uiConfig?.emitDataMasks) { - log('setting up Switchboard event emitter'); - - let previousDataMask = store.getState().dataMask; - - store.subscribe(() => { - const currentState = store.getState(); - const currentDataMask = currentState.dataMask; - - // Only emit if the dataMask has changed - if (previousDataMask !== currentDataMask) { - Switchboard.emit('observeDataMask', { - ...currentDataMask, - ...getDataMaskChangeTrigger(currentDataMask, previousDataMask), - }); - previousDataMask = currentDataMask; - } - }); - } - - return <LazyDashboardPage idOrSlug={bootstrapData.embedded!.dashboard_id} />; + makeApi, + SupersetApiError, + getExtensionsRegistry, +} from '@superset-ui/core'; +import { styled, css, Alert } from '@apache-superset/core/ui'; +import { + Button, + FormItem, + InfoTooltip, + Input, + Modal, + Loading, + Form, + Space, +} from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { EmbeddedDashboard } from 'src/dashboard/types'; +import { Typography } from '@superset-ui/core/components/Typography'; +import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; + +const extensionsRegistry = getExtensionsRegistry(); + +type Props = { + dashboardId: string; + show: boolean; + onHide: () => void; }; -const EmbeddedRoute = () => ( - <EmbeddedContextProviders> - <Suspense fallback={<Loading />}> - <ErrorBoundary> - <EmbededLazyDashboardPage /> - </ErrorBoundary> - <ToastContainer position="top" /> - </Suspense> - </EmbeddedContextProviders> -); - -const EmbeddedApp = () => ( - <Router basename={applicationRoot()}> - {/* todo (embedded) remove this line after uuids are deployed */} - <Route path="/dashboard/:idOrSlug/embedded/" component={EmbeddedRoute} /> - <Route path="/embedded/:uuid/" component={EmbeddedRoute} /> - </Router> -); - -const appMountPoint = document.getElementById('app')!; - -const MESSAGE_TYPE = '__embedded_comms__'; - -function showFailureMessage(message: string) { - appMountPoint.innerHTML = message; -} - -if (!window.parent || window.parent === window) { - showFailureMessage( - t( - 'This page is intended to be embedded in an iframe, but it looks like that is not the case.', - ), - ); -} - -// if the page is embedded in an origin that hasn't -// been authorized by the curator, we forbid access entirely. -// todo: check the referrer on the route serving this page instead -// const ALLOW_ORIGINS = ['http://127.0.0.1:9001', 'http://localhost:9001']; -// const parentOrigin = new URL(document.referrer).origin; -// if (!ALLOW_ORIGINS.includes(parentOrigin)) { -// throw new Error( -// `[superset] iframe parent ${parentOrigin} is not in the list of allowed origins`, -// ); -// } - -let displayedUnauthorizedToast = false; - -/** - * If there is a problem with the guest token, we will start getting - * 401 errors from the api and SupersetClient will call this function. - */ -function guestUnauthorizedHandler() { - if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401 - displayedUnauthorizedToast = true; - // If a guest user were sent to a login screen on 401, they would have no valid login to use. - // For embedded it makes more sense to just display a message - // and let them continue accessing the page, to whatever extent they can. - store.dispatch( - addDangerToast( - t( - 'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.', - ), - { - duration: -1, // stay open until manually closed - noDuplicate: true, - }, - ), - ); -} - -function start() { - const getMeWithRole = makeApi<void, { result: UserWithPermissionsAndRoles }>({ - method: 'GET', - endpoint: '/api/v1/me/roles/', - }); - return getMeWithRole().then( - ({ result }) => { - // fill in some missing bootstrap data - // (because at pageload, we don't have any auth yet) - // this allows the frontend's permissions checks to work. - bootstrapData.user = result; - store.dispatch({ - type: USER_LOADED, - user: result, +type EmbeddedApiPayload = { allowed_domains: string[] }; + +const stringToList = (stringyList: string): string[] => + stringyList.split(/(?:\s|,)+/).filter(x => x); + +const ButtonRow = styled.div` + display: flex; + flex-direction: row; + justify-content: flex-end; +`; + +export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => { + const { addInfoToast, addDangerToast } = useToasts(); + const [ready, setReady] = useState(true); // whether we have initialized yet + const [loading, setLoading] = useState(false); // whether we are currently doing an async thing + const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); // the embedded dashboard config + const [allowedDomains, setAllowedDomains] = useState<string>(''); + const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false); + + const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`; + // whether saveable changes have been made to the config + const isDirty = + !embedded || + stringToList(allowedDomains).join() !== embedded.allowed_domains.join(); + + const enableEmbedded = useCallback(() => { + setLoading(true); + makeApi<EmbeddedApiPayload, { result: EmbeddedDashboard }>({ + method: 'POST', + endpoint, + })({ + allowed_domains: stringToList(allowedDomains), + }) + .then( + ({ result }) => { + setEmbedded(result); + setAllowedDomains(result.allowed_domains.join(', ')); + addInfoToast(t('Changes saved.')); + }, + err => { + console.error(err); + addDangerToast( + t( + t('Sorry, something went wrong. The changes could not be saved.'), + ), + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Double translation call</b></div> <div id="fix"> 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. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion addDangerToast( t('Sorry, something went wrong. The changes could not be saved.'), ); ```` </div> </details> </div> <small><i>Code Review Run #c80ea8</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/components/OnboardingWorkflows/index.tsx: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ComponentProps } from 'react'; +import useShouldRenderOnboardingWorkflow from './useShouldRenderOnboardingWorkflow'; +import Joyride, { ACTIONS, CallBackProps, ORIGIN, STATUS } from 'react-joyride'; +import { useDispatch, useSelector } from 'react-redux'; +import UserOnboardingWorkflowsState from 'src/userOnboardingWorkflow/types'; +import { markCurrentUserOnboardingWorkflowAsVisited } from 'src/userOnboardingWorkflow/actions'; + +type OnboardingWorkflowProps = { + onboardginWorkflowName: string; + run?: ComponentProps<typeof Joyride>['run']; + steps: ComponentProps<typeof Joyride>['steps']; + locale?: ComponentProps<typeof Joyride>['locale']; + stepIndex?: ComponentProps<typeof Joyride>['stepIndex']; + callback?: ComponentProps<typeof Joyride>['callback']; + styles?: ComponentProps<typeof Joyride>['styles']; +}; + +export default function OnboardingWorkflow({ + onboardginWorkflowName, + run, + steps, + locale, + stepIndex, + callback, + styles, +}: OnboardingWorkflowProps) { + const dispatch = useDispatch(); + const userOnboardingWorkflowNamesMap = useSelector( + (state: { userOnboardingWorkflows: UserOnboardingWorkflowsState }) => + state.userOnboardingWorkflows.userOnboardingWorkflowNamesMap || {}, + ); + + const shouldRenderOnboardingWorkflow = useShouldRenderOnboardingWorkflow( + onboardginWorkflowName, + ); + + if (!shouldRenderOnboardingWorkflow) { + return null; + } + + const handleJoyrideCallback = (data: CallBackProps) => { + callback?.(data); + + const { status, action } = data; + + if ( + (ORIGIN.BUTTON_CLOSE && action === ACTIONS.CLOSE) || + status === STATUS.FINISHED Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete callback destructuring and faulty condition</b></div> <div id="fix"> 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). </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const { status, action, origin } = data; if ( (origin === ORIGIN.BUTTON_CLOSE && action === ACTIONS.CLOSE) || status === STATUS.FINISHED ```` </div> </details> </div> <small><i>Code Review Run #c80ea8</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]
