refactor(web): make account setting fully controlled with action props

2026-03-05 13:39:36 +08:00 · 2026-03-05 13:39:36 +08:00 · 80e9c8bee0
parent 15b7b304d2
commit 80e9c8bee0
6 changed files with 82 additions and 87 deletions
--- a/web/app/components/datasets/documents/detail/settings/tests/document-settings.spec.tsx
+++ b/web/app/components/datasets/documents/detail/settings/tests/document-settings.spec.tsx
@ -100,10 +100,10 @@ vi.mock('@/app/components/datasets/create/step-two', () => ({
 }))

 vi.mock('@/app/components/header/account-setting', () => ({
-  default: ({ activeTab, onCancel }: { activeTab?: string, onCancel?: () => void }) => (
+  default: ({ activeTab, onCancelAction }: { activeTab?: string, onCancelAction?: () => void }) => (
    <div data-testid="account-setting">
      <span data-testid="active-tab">{activeTab}</span>
-      <button onClick={onCancel} data-testid="close-setting">Close</button>
+      <button onClick={onCancelAction} data-testid="close-setting">Close</button>
    </div>
  ),
 }))
--- a/web/app/components/datasets/documents/detail/settings/document-settings.tsx
+++ b/web/app/components/datasets/documents/detail/settings/document-settings.tsx
@ -1,3 +1,4 @@
+import type { AccountSettingTab } from '@/app/components/header/account-setting/constants'
 import type { DataSourceProvider, NotionPage } from '@/models/common'
 import type {
  CrawlOptions,
@ -19,6 +20,7 @@ import AppUnavailable from '@/app/components/base/app-unavailable'
 import Loading from '@/app/components/base/loading'
 import StepTwo from '@/app/components/datasets/create/step-two'
 import AccountSetting from '@/app/components/header/account-setting'
+import { ACCOUNT_SETTING_TAB } from '@/app/components/header/account-setting/constants'
 import { ModelTypeEnum } from '@/app/components/header/account-setting/model-provider-page/declarations'
 import { useDefaultModel } from '@/app/components/header/account-setting/model-provider-page/hooks'
 import DatasetDetailContext from '@/context/dataset-detail'
@ -33,8 +35,13 @@ const DocumentSettings = ({ datasetId, documentId }: DocumentSettingsProps) => {
  const { t } = useTranslation()
  const router = useRouter()
  const [isShowSetAPIKey, { setTrue: showSetAPIKey, setFalse: hideSetAPIkey }] = useBoolean()
+  const [accountSettingTab, setAccountSettingTab] = React.useState<AccountSettingTab>(ACCOUNT_SETTING_TAB.PROVIDER)
  const { indexingTechnique, dataset } = useContext(DatasetDetailContext)
  const { data: embeddingsDefaultModel } = useDefaultModel(ModelTypeEnum.textEmbedding)
+  const handleOpenAccountSetting = React.useCallback(() => {
+    setAccountSettingTab(ACCOUNT_SETTING_TAB.PROVIDER)
+    showSetAPIKey()
+  }, [showSetAPIKey])

  const invalidDocumentList = useInvalidDocumentList(datasetId)
  const invalidDocumentDetail = useInvalidDocumentDetail()
@ -135,7 +142,7 @@ const DocumentSettings = ({ datasetId, documentId }: DocumentSettingsProps) => {
        {dataset && documentDetail && (
          <StepTwo
            isAPIKeySet={!!embeddingsDefaultModel}
-            onSetting={showSetAPIKey}
+            onSetting={handleOpenAccountSetting}
            datasetId={datasetId}
            dataSourceType={documentDetail.data_source_type as DataSourceType}
            notionPages={currentPage ? [currentPage as unknown as NotionPage] : []}
@ -155,8 +162,9 @@ const DocumentSettings = ({ datasetId, documentId }: DocumentSettingsProps) => {
      </div>
      {isShowSetAPIKey && (
        <AccountSetting
-          activeTab="provider"
-          onCancel={async () => {
+          activeTab={accountSettingTab}
+          onTabChangeAction={setAccountSettingTab}
+          onCancelAction={async () => {
            hideSetAPIkey()
          }}
        />
--- a/web/app/components/header/account-setting/index.spec.tsx
+++ b/web/app/components/header/account-setting/index.spec.tsx
@ -1,6 +1,8 @@
+import type { AccountSettingTab } from './constants'
 import type { AppContextValue } from '@/context/app-context'
 import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
 import { fireEvent, render, screen } from '@testing-library/react'
+import { useState } from 'react'
 import { useAppContext } from '@/context/app-context'
 import { baseProviderContextValue, useProviderContext } from '@/context/provider-context'
 import useBreakpoints, { MediaType } from '@/hooks/use-breakpoints'
@ -112,6 +114,38 @@ const baseAppContextValue: AppContextValue = {
 describe('AccountSetting', () => {
  const mockOnCancel = vi.fn()
  const mockOnTabChange = vi.fn()
+  const renderAccountSetting = (props?: {
+    initialTab?: AccountSettingTab
+    onCancel?: () => void
+    onTabChange?: (tab: AccountSettingTab) => void
+  }) => {
+    const {
+      initialTab = ACCOUNT_SETTING_TAB.MEMBERS,
+      onCancel = mockOnCancel,
+      onTabChange = mockOnTabChange,
+    } = props ?? {}
+
+    const StatefulAccountSetting = () => {
+      const [activeTab, setActiveTab] = useState<AccountSettingTab>(initialTab)
+
+      return (
+        <AccountSetting
+          onCancelAction={onCancel}
+          activeTab={activeTab}
+          onTabChangeAction={(tab) => {
+            setActiveTab(tab)
+            onTabChange(tab)
+          }}
+        />
+      )
+    }
+
+    return render(
+      <QueryClientProvider client={new QueryClient()}>
+        <StatefulAccountSetting />
+      </QueryClientProvider>,
+    )
+  }

  beforeEach(() => {
    vi.clearAllMocks()
@ -127,11 +161,7 @@ describe('AccountSetting', () => {
  describe('Rendering', () => {
    it('should render the sidebar with correct menu items', () => {
      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()

      // Assert
      expect(screen.getByText('common.userProfile.settings')).toBeInTheDocument()
@ -144,13 +174,9 @@ describe('AccountSetting', () => {
      expect(screen.getAllByText('common.settings.language').length).toBeGreaterThan(0)
    })

-    it('should respect the activeTab prop', () => {
+    it('should respect the initial tab', () => {
      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} activeTab={ACCOUNT_SETTING_TAB.DATA_SOURCE} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting({ initialTab: ACCOUNT_SETTING_TAB.DATA_SOURCE })

      // Assert
      // Check that the active item title is Data Source
@ -164,11 +190,7 @@ describe('AccountSetting', () => {
      vi.mocked(useBreakpoints).mockReturnValue(MediaType.mobile)

      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()

      // Assert
      // On mobile, the labels should not be rendered as per the implementation
@ -183,11 +205,7 @@ describe('AccountSetting', () => {
      })

      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()

      // Assert
      expect(screen.queryByText('common.settings.provider')).not.toBeInTheDocument()
@ -204,11 +222,7 @@ describe('AccountSetting', () => {
      })

      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()

      // Assert
      expect(screen.queryByText('common.settings.billing')).not.toBeInTheDocument()
@ -219,11 +233,7 @@ describe('AccountSetting', () => {
  describe('Tab Navigation', () => {
    it('should change active tab when clicking on menu item', () => {
      // Arrange
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} onTabChange={mockOnTabChange} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting({ onTabChange: mockOnTabChange })

      // Act
      fireEvent.click(screen.getByText('common.settings.provider'))
@ -236,11 +246,7 @@ describe('AccountSetting', () => {

    it('should navigate through various tabs and show correct details', () => {
      // Act & Assert
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()

      // Billing
      fireEvent.click(screen.getByText('common.settings.billing'))
@ -274,11 +280,7 @@ describe('AccountSetting', () => {
  describe('Interactions', () => {
    it('should call onCancel when clicking close button', () => {
      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()
      const closeIcon = document.querySelector('.i-ri-close-line')
      const closeButton = closeIcon?.closest('button')
      expect(closeButton).not.toBeNull()
@ -290,11 +292,7 @@ describe('AccountSetting', () => {

    it('should call onCancel when pressing Escape key', () => {
      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()
      fireEvent.keyDown(document, { key: 'Escape' })

      // Assert
@ -303,12 +301,7 @@ describe('AccountSetting', () => {

    it('should update search value in provider tab', () => {
      // Arrange
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
-      fireEvent.click(screen.getByText('common.settings.provider'))
+      renderAccountSetting({ initialTab: ACCOUNT_SETTING_TAB.PROVIDER })

      // Act
      const input = screen.getByRole('textbox')
@ -321,11 +314,7 @@ describe('AccountSetting', () => {

    it('should handle scroll event in panel', () => {
      // Act
-      render(
-        <QueryClientProvider client={new QueryClient()}>
-          <AccountSetting onCancel={mockOnCancel} />
-        </QueryClientProvider>,
-      )
+      renderAccountSetting()
      const scrollContainer = screen.getByRole('dialog').querySelector('.overflow-y-auto')

      // Assert
--- a/web/app/components/header/account-setting/index.tsx
+++ b/web/app/components/header/account-setting/index.tsx
@ -27,9 +27,9 @@ const iconClassName = `
 `

 type IAccountSettingProps = {
-  onCancel: () => void
-  activeTab?: AccountSettingTab
-  onTabChange?: (tab: AccountSettingTab) => void
+  onCancelAction: () => void
+  activeTab: AccountSettingTab
+  onTabChangeAction: (tab: AccountSettingTab) => void
 }

 type GroupItem = {
@ -41,16 +41,12 @@ type GroupItem = {
 }

 export default function AccountSetting({
-  onCancel,
+  onCancelAction,
  activeTab,
-  onTabChange,
+  onTabChangeAction,
 }: IAccountSettingProps) {
  const resetModelProviderListExpanded = useResetModelProviderListExpanded()
-  const isControlledTab = activeTab !== undefined && !!onTabChange
-  const [uncontrolledActiveMenu, setUncontrolledActiveMenu] = useState<AccountSettingTab>(activeTab ?? ACCOUNT_SETTING_TAB.MEMBERS)
-  const activeMenu = isControlledTab
-    ? (activeTab ?? ACCOUNT_SETTING_TAB.MEMBERS)
-    : uncontrolledActiveMenu
+  const activeMenu = activeTab
  const { t } = useTranslation()
  const { enableBilling, enableReplaceWebAppLogo } = useProviderContext()
  const { isCurrentWorkspaceDatasetOperator } = useAppContext()
@ -155,16 +151,13 @@ export default function AccountSetting({
    if (tab === ACCOUNT_SETTING_TAB.PROVIDER)
      resetModelProviderListExpanded()

-    if (!isControlledTab)
-      setUncontrolledActiveMenu(tab)
-
-    onTabChange?.(tab)
-  }, [isControlledTab, onTabChange, resetModelProviderListExpanded])
+    onTabChangeAction(tab)
+  }, [onTabChangeAction, resetModelProviderListExpanded])

  const handleClose = useCallback(() => {
    resetModelProviderListExpanded()
-    onCancel()
-  }, [onCancel, resetModelProviderListExpanded])
+    onCancelAction()
+  }, [onCancelAction, resetModelProviderListExpanded])

  return (
    <MenuDialog
@ -184,12 +177,14 @@ export default function AccountSetting({
                  <div>
                    {
                      menuItem.items.map(item => (
-                        <div
+                        <button
+                          type="button"
                          key={item.key}
                          className={cn(
-                            'mb-0.5 flex h-[37px] cursor-pointer items-center rounded-lg p-1 pl-3 text-sm',
+                            'mb-0.5 flex h-[37px] w-full items-center rounded-lg p-1 pl-3 text-left text-sm',
                            activeMenu === item.key ? 'bg-state-base-active text-components-menu-item-text-active system-sm-semibold' : 'text-components-menu-item-text system-sm-medium',
                          )}
+                          aria-label={item.name}
                          title={item.name}
                          onClick={() => {
                            handleTabChange(item.key)
@ -197,7 +192,7 @@ export default function AccountSetting({
                        >
                          {activeMenu === item.key ? item.activeIcon : item.icon}
                          {!isMobile && <div className="truncate">{item.name}</div>}
-                        </div>
+                        </button>
                      ))
                    }
                  </div>
@ -212,6 +207,7 @@ export default function AccountSetting({
              variant="tertiary"
              size="large"
              className="px-2"
+              aria-label={t('operation.close', { ns: 'common' })}
              onClick={handleClose}
            >
              <span className="i-ri-close-line h-5 w-5" />
--- a/web/app/components/header/account-setting/model-provider-page/provider-added-card/index.tsx
+++ b/web/app/components/header/account-setting/model-provider-page/provider-added-card/index.tsx
@ -125,9 +125,11 @@ const ProviderAddedCard: FC<ProviderAddedCardProps> = ({
        showCollapsedSection && (
          <div className="group flex items-center justify-between border-t border-t-divider-subtle py-1.5 pl-2 pr-[11px] text-text-tertiary system-xs-medium">
            {(showModelProvider || !notConfigured) && (
-              <div
+              <button
+                type="button"
                data-testid="show-models-button"
-                className="flex h-6 cursor-pointer items-center rounded-lg pl-1 pr-1.5 hover:bg-components-button-ghost-bg-hover"
+                className="flex h-6 items-center rounded-lg pl-1 pr-1.5 hover:bg-components-button-ghost-bg-hover"
+                aria-label={t('modelProvider.showModels', { ns: 'common' })}
                onClick={handleOpenModelList}
              >
                {
@ -141,7 +143,7 @@ const ProviderAddedCard: FC<ProviderAddedCardProps> = ({
                    <div className="i-ri-loader-2-line ml-0.5 h-3 w-3 animate-spin" />
                  )
                }
-              </div>
+              </button>
            )}
            {!showModelProvider && notConfigured && (
              <div className="flex h-6 items-center pl-1 pr-1.5">
--- a/web/context/modal-context.tsx
+++ b/web/context/modal-context.tsx
@ -343,8 +343,8 @@ export const ModalContextProvider = ({
          accountSettingTab && (
            <AccountSetting
              activeTab={accountSettingTab}
-              onCancel={handleCancelAccountSettingModal}
-              onTabChange={handleAccountSettingTabChange}
+              onCancelAction={handleCancelAccountSettingModal}
+              onTabChangeAction={handleAccountSettingTabChange}
            />
          )
        }