From ba12960975ab4ed750bb0b7e737d96a60829f835 Mon Sep 17 00:00:00 2001 From: yyh <92089059+lyzno1@users.noreply.github.com> Date: Sat, 14 Feb 2026 17:31:37 +0800 Subject: [PATCH] refactor(web): centralize role-based route guards and fix anti-patterns (#32302) --- .../apps/app-list-browsing-flow.test.tsx | 8 +- .../app/(appDetailLayout)/layout.tsx | 10 -- .../(commonLayout)/datasets/layout.spec.tsx | 108 +++++++++++++++++ web/app/(commonLayout)/datasets/layout.tsx | 16 ++- web/app/(commonLayout)/layout.tsx | 5 +- .../(commonLayout)/role-route-guard.spec.tsx | 109 ++++++++++++++++++ web/app/(commonLayout)/role-route-guard.tsx | 33 ++++++ web/app/(commonLayout)/tools/page.tsx | 10 -- .../components/apps/__tests__/list.spec.tsx | 6 +- web/app/components/apps/list.tsx | 41 ++----- .../datasets/list/__tests__/index.spec.tsx | 4 +- web/app/components/datasets/list/index.tsx | 13 +-- .../explore/__tests__/index.spec.tsx | 4 +- web/app/components/explore/index.tsx | 11 -- web/eslint-suppressions.json | 10 -- 15 files changed, 286 insertions(+), 102 deletions(-) create mode 100644 web/app/(commonLayout)/datasets/layout.spec.tsx create mode 100644 web/app/(commonLayout)/role-route-guard.spec.tsx create mode 100644 web/app/(commonLayout)/role-route-guard.tsx diff --git a/web/__tests__/apps/app-list-browsing-flow.test.tsx b/web/__tests__/apps/app-list-browsing-flow.test.tsx index 9450d13670..1c046f5dd0 100644 --- a/web/__tests__/apps/app-list-browsing-flow.test.tsx +++ b/web/__tests__/apps/app-list-browsing-flow.test.tsx @@ -390,13 +390,13 @@ describe('App List Browsing Flow', () => { }) }) - // -- Dataset operator redirect -- - describe('Dataset Operator Redirect', () => { - it('should redirect dataset operators to /datasets', () => { + // -- Dataset operator behavior -- + describe('Dataset Operator Behavior', () => { + it('should not redirect at list component level for dataset operators', () => { mockIsCurrentWorkspaceDatasetOperator = true renderList() - expect(mockRouterReplace).toHaveBeenCalledWith('/datasets') + expect(mockRouterReplace).not.toHaveBeenCalled() }) }) diff --git a/web/app/(commonLayout)/app/(appDetailLayout)/layout.tsx b/web/app/(commonLayout)/app/(appDetailLayout)/layout.tsx index a918ae2786..f79ca6cfcc 100644 --- a/web/app/(commonLayout)/app/(appDetailLayout)/layout.tsx +++ b/web/app/(commonLayout)/app/(appDetailLayout)/layout.tsx @@ -1,10 +1,7 @@ 'use client' import type { FC } from 'react' -import { useRouter } from 'next/navigation' import * as React from 'react' -import { useEffect } from 'react' import { useTranslation } from 'react-i18next' -import { useAppContext } from '@/context/app-context' import useDocumentTitle from '@/hooks/use-document-title' export type IAppDetail = { @@ -12,16 +9,9 @@ export type IAppDetail = { } const AppDetail: FC = ({ children }) => { - const router = useRouter() - const { isCurrentWorkspaceDatasetOperator } = useAppContext() const { t } = useTranslation() useDocumentTitle(t('menus.appDetail', { ns: 'common' })) - useEffect(() => { - if (isCurrentWorkspaceDatasetOperator) - return router.replace('/datasets') - }, [isCurrentWorkspaceDatasetOperator, router]) - return ( <> {children} diff --git a/web/app/(commonLayout)/datasets/layout.spec.tsx b/web/app/(commonLayout)/datasets/layout.spec.tsx new file mode 100644 index 0000000000..5873f344d0 --- /dev/null +++ b/web/app/(commonLayout)/datasets/layout.spec.tsx @@ -0,0 +1,108 @@ +import type { ReactNode } from 'react' +import { render, screen, waitFor } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import DatasetsLayout from './layout' + +const mockReplace = vi.fn() +const mockUseAppContext = vi.fn() + +vi.mock('next/navigation', () => ({ + useRouter: () => ({ + replace: mockReplace, + }), +})) + +vi.mock('@/context/app-context', () => ({ + useAppContext: () => mockUseAppContext(), +})) + +vi.mock('@/context/external-api-panel-context', () => ({ + ExternalApiPanelProvider: ({ children }: { children: ReactNode }) => <>{children}, +})) + +vi.mock('@/context/external-knowledge-api-context', () => ({ + ExternalKnowledgeApiProvider: ({ children }: { children: ReactNode }) => <>{children}, +})) + +type AppContextMock = { + isCurrentWorkspaceEditor: boolean + isCurrentWorkspaceDatasetOperator: boolean + isLoadingCurrentWorkspace: boolean + currentWorkspace: { + id: string + } +} + +const baseContext: AppContextMock = { + isCurrentWorkspaceEditor: true, + isCurrentWorkspaceDatasetOperator: false, + isLoadingCurrentWorkspace: false, + currentWorkspace: { + id: 'workspace-1', + }, +} + +const setAppContext = (overrides: Partial = {}) => { + mockUseAppContext.mockReturnValue({ + ...baseContext, + ...overrides, + }) +} + +describe('DatasetsLayout', () => { + beforeEach(() => { + vi.clearAllMocks() + setAppContext() + }) + + it('should render loading when workspace is still loading', () => { + setAppContext({ + isLoadingCurrentWorkspace: true, + currentWorkspace: { id: '' }, + }) + + render(( + +
datasets
+
+ )) + + expect(screen.getByRole('status')).toBeInTheDocument() + expect(screen.queryByTestId('datasets-content')).not.toBeInTheDocument() + expect(mockReplace).not.toHaveBeenCalled() + }) + + it('should redirect non-editor and non-dataset-operator users to /apps', async () => { + setAppContext({ + isCurrentWorkspaceEditor: false, + isCurrentWorkspaceDatasetOperator: false, + }) + + render(( + +
datasets
+
+ )) + + expect(screen.queryByTestId('datasets-content')).not.toBeInTheDocument() + await waitFor(() => { + expect(mockReplace).toHaveBeenCalledWith('/apps') + }) + }) + + it('should render children for dataset operators', () => { + setAppContext({ + isCurrentWorkspaceEditor: false, + isCurrentWorkspaceDatasetOperator: true, + }) + + render(( + +
datasets
+
+ )) + + expect(screen.getByTestId('datasets-content')).toBeInTheDocument() + expect(mockReplace).not.toHaveBeenCalled() + }) +}) diff --git a/web/app/(commonLayout)/datasets/layout.tsx b/web/app/(commonLayout)/datasets/layout.tsx index fda4d3c803..b543c42570 100644 --- a/web/app/(commonLayout)/datasets/layout.tsx +++ b/web/app/(commonLayout)/datasets/layout.tsx @@ -10,16 +10,22 @@ import { ExternalKnowledgeApiProvider } from '@/context/external-knowledge-api-c export default function DatasetsLayout({ children }: { children: React.ReactNode }) { const { isCurrentWorkspaceEditor, isCurrentWorkspaceDatasetOperator, currentWorkspace, isLoadingCurrentWorkspace } = useAppContext() const router = useRouter() + const shouldRedirect = !isLoadingCurrentWorkspace + && currentWorkspace.id + && !(isCurrentWorkspaceEditor || isCurrentWorkspaceDatasetOperator) useEffect(() => { - if (isLoadingCurrentWorkspace || !currentWorkspace.id) - return - if (!(isCurrentWorkspaceEditor || isCurrentWorkspaceDatasetOperator)) + if (shouldRedirect) router.replace('/apps') - }, [isCurrentWorkspaceEditor, isCurrentWorkspaceDatasetOperator, isLoadingCurrentWorkspace, currentWorkspace, router]) + }, [shouldRedirect, router]) - if (isLoadingCurrentWorkspace || !(isCurrentWorkspaceEditor || isCurrentWorkspaceDatasetOperator)) + if (isLoadingCurrentWorkspace || !currentWorkspace.id) return + + if (shouldRedirect) { + return null + } + return ( diff --git a/web/app/(commonLayout)/layout.tsx b/web/app/(commonLayout)/layout.tsx index a0ccde957d..abd5dd96fd 100644 --- a/web/app/(commonLayout)/layout.tsx +++ b/web/app/(commonLayout)/layout.tsx @@ -14,6 +14,7 @@ import { ModalContextProvider } from '@/context/modal-context' import { ProviderContextProvider } from '@/context/provider-context' import PartnerStack from '../components/billing/partner-stack' import Splash from '../components/splash' +import RoleRouteGuard from './role-route-guard' const Layout = ({ children }: { children: ReactNode }) => { return ( @@ -28,7 +29,9 @@ const Layout = ({ children }: { children: ReactNode }) => {
- {children} + + {children} + diff --git a/web/app/(commonLayout)/role-route-guard.spec.tsx b/web/app/(commonLayout)/role-route-guard.spec.tsx new file mode 100644 index 0000000000..87bf9be8af --- /dev/null +++ b/web/app/(commonLayout)/role-route-guard.spec.tsx @@ -0,0 +1,109 @@ +import { render, screen, waitFor } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import RoleRouteGuard from './role-route-guard' + +const mockReplace = vi.fn() +const mockUseAppContext = vi.fn() +let mockPathname = '/apps' + +vi.mock('next/navigation', () => ({ + usePathname: () => mockPathname, + useRouter: () => ({ + replace: mockReplace, + }), +})) + +vi.mock('@/context/app-context', () => ({ + useAppContext: () => mockUseAppContext(), +})) + +type AppContextMock = { + isCurrentWorkspaceDatasetOperator: boolean + isLoadingCurrentWorkspace: boolean +} + +const baseContext: AppContextMock = { + isCurrentWorkspaceDatasetOperator: false, + isLoadingCurrentWorkspace: false, +} + +const setAppContext = (overrides: Partial = {}) => { + mockUseAppContext.mockReturnValue({ + ...baseContext, + ...overrides, + }) +} + +describe('RoleRouteGuard', () => { + beforeEach(() => { + vi.clearAllMocks() + mockPathname = '/apps' + setAppContext() + }) + + it('should render loading while workspace is loading', () => { + setAppContext({ + isLoadingCurrentWorkspace: true, + }) + + render(( + +
content
+
+ )) + + expect(screen.getByRole('status')).toBeInTheDocument() + expect(screen.queryByTestId('guarded-content')).not.toBeInTheDocument() + expect(mockReplace).not.toHaveBeenCalled() + }) + + it('should redirect dataset operator on guarded routes', async () => { + setAppContext({ + isCurrentWorkspaceDatasetOperator: true, + }) + + render(( + +
content
+
+ )) + + expect(screen.queryByTestId('guarded-content')).not.toBeInTheDocument() + await waitFor(() => { + expect(mockReplace).toHaveBeenCalledWith('/datasets') + }) + }) + + it('should allow dataset operator on non-guarded routes', () => { + mockPathname = '/plugins' + setAppContext({ + isCurrentWorkspaceDatasetOperator: true, + }) + + render(( + +
content
+
+ )) + + expect(screen.getByTestId('guarded-content')).toBeInTheDocument() + expect(mockReplace).not.toHaveBeenCalled() + }) + + it('should not block non-guarded routes while workspace is loading', () => { + mockPathname = '/plugins' + setAppContext({ + isLoadingCurrentWorkspace: true, + }) + + render(( + +
content
+
+ )) + + expect(screen.getByTestId('guarded-content')).toBeInTheDocument() + expect(screen.queryByRole('status')).not.toBeInTheDocument() + expect(mockReplace).not.toHaveBeenCalled() + }) +}) diff --git a/web/app/(commonLayout)/role-route-guard.tsx b/web/app/(commonLayout)/role-route-guard.tsx new file mode 100644 index 0000000000..1c42be9d15 --- /dev/null +++ b/web/app/(commonLayout)/role-route-guard.tsx @@ -0,0 +1,33 @@ +'use client' + +import type { ReactNode } from 'react' +import { usePathname, useRouter } from 'next/navigation' +import { useEffect } from 'react' +import Loading from '@/app/components/base/loading' +import { useAppContext } from '@/context/app-context' + +const datasetOperatorRedirectRoutes = ['/apps', '/app', '/explore', '/tools'] as const + +const isPathUnderRoute = (pathname: string, route: string) => pathname === route || pathname.startsWith(`${route}/`) + +export default function RoleRouteGuard({ children }: { children: ReactNode }) { + const { isCurrentWorkspaceDatasetOperator, isLoadingCurrentWorkspace } = useAppContext() + const pathname = usePathname() + const router = useRouter() + const shouldGuardRoute = datasetOperatorRedirectRoutes.some(route => isPathUnderRoute(pathname, route)) + const shouldRedirect = shouldGuardRoute && !isLoadingCurrentWorkspace && isCurrentWorkspaceDatasetOperator + + useEffect(() => { + if (shouldRedirect) + router.replace('/datasets') + }, [shouldRedirect, router]) + + // Block rendering only for guarded routes to avoid permission flicker. + if (shouldGuardRoute && isLoadingCurrentWorkspace) + return + + if (shouldRedirect) + return null + + return <>{children} +} diff --git a/web/app/(commonLayout)/tools/page.tsx b/web/app/(commonLayout)/tools/page.tsx index 3e88050eba..be8344660d 100644 --- a/web/app/(commonLayout)/tools/page.tsx +++ b/web/app/(commonLayout)/tools/page.tsx @@ -1,24 +1,14 @@ 'use client' import type { FC } from 'react' -import { useRouter } from 'next/navigation' import * as React from 'react' -import { useEffect } from 'react' import { useTranslation } from 'react-i18next' import ToolProviderList from '@/app/components/tools/provider-list' -import { useAppContext } from '@/context/app-context' import useDocumentTitle from '@/hooks/use-document-title' const ToolsList: FC = () => { - const router = useRouter() - const { isCurrentWorkspaceDatasetOperator } = useAppContext() const { t } = useTranslation() useDocumentTitle(t('menus.tools', { ns: 'common' })) - useEffect(() => { - if (isCurrentWorkspaceDatasetOperator) - return router.replace('/datasets') - }, [isCurrentWorkspaceDatasetOperator, router]) - return } export default React.memo(ToolsList) diff --git a/web/app/components/apps/__tests__/list.spec.tsx b/web/app/components/apps/__tests__/list.spec.tsx index 2d4013012f..fa83296267 100644 --- a/web/app/components/apps/__tests__/list.spec.tsx +++ b/web/app/components/apps/__tests__/list.spec.tsx @@ -368,13 +368,13 @@ describe('List', () => { }) }) - describe('Dataset Operator Redirect', () => { - it('should redirect dataset operators to datasets page', () => { + describe('Dataset Operator Behavior', () => { + it('should not trigger redirect at component level for dataset operators', () => { mockIsCurrentWorkspaceDatasetOperator.mockReturnValue(true) renderList() - expect(mockReplace).toHaveBeenCalledWith('/datasets') + expect(mockReplace).not.toHaveBeenCalled() }) }) diff --git a/web/app/components/apps/list.tsx b/web/app/components/apps/list.tsx index 6bf79b7338..d97cd176ca 100644 --- a/web/app/components/apps/list.tsx +++ b/web/app/components/apps/list.tsx @@ -1,19 +1,8 @@ 'use client' import type { FC } from 'react' -import { - RiApps2Line, - RiDragDropLine, - RiExchange2Line, - RiFile4Line, - RiMessage3Line, - RiRobot3Line, -} from '@remixicon/react' import { useDebounceFn } from 'ahooks' import dynamic from 'next/dynamic' -import { - useRouter, -} from 'next/navigation' import { parseAsString, useQueryState } from 'nuqs' import { useCallback, useEffect, useRef, useState } from 'react' import { useTranslation } from 'react-i18next' @@ -37,16 +26,6 @@ import useAppsQueryState from './hooks/use-apps-query-state' import { useDSLDragDrop } from './hooks/use-dsl-drag-drop' import NewAppCard from './new-app-card' -// Define valid tabs at module scope to avoid re-creation on each render and stale closures -const validTabs = new Set([ - 'all', - AppModeEnum.WORKFLOW, - AppModeEnum.ADVANCED_CHAT, - AppModeEnum.CHAT, - AppModeEnum.AGENT_CHAT, - AppModeEnum.COMPLETION, -]) - const TagManagementModal = dynamic(() => import('@/app/components/base/tag-management'), { ssr: false, }) @@ -62,7 +41,6 @@ const List: FC = ({ }) => { const { t } = useTranslation() const { systemFeatures } = useGlobalPublicStore() - const router = useRouter() const { isCurrentWorkspaceEditor, isCurrentWorkspaceDatasetOperator, isLoadingCurrentWorkspace } = useAppContext() const showTagManagementModal = useTagStore(s => s.showTagManagementModal) const [activeTab, setActiveTab] = useQueryState( @@ -125,12 +103,12 @@ const List: FC = ({ const anchorRef = useRef(null) const options = [ - { value: 'all', text: t('types.all', { ns: 'app' }), icon: }, - { value: AppModeEnum.WORKFLOW, text: t('types.workflow', { ns: 'app' }), icon: }, - { value: AppModeEnum.ADVANCED_CHAT, text: t('types.advanced', { ns: 'app' }), icon: }, - { value: AppModeEnum.CHAT, text: t('types.chatbot', { ns: 'app' }), icon: }, - { value: AppModeEnum.AGENT_CHAT, text: t('types.agent', { ns: 'app' }), icon: }, - { value: AppModeEnum.COMPLETION, text: t('types.completion', { ns: 'app' }), icon: }, + { value: 'all', text: t('types.all', { ns: 'app' }), icon: }, + { value: AppModeEnum.WORKFLOW, text: t('types.workflow', { ns: 'app' }), icon: }, + { value: AppModeEnum.ADVANCED_CHAT, text: t('types.advanced', { ns: 'app' }), icon: }, + { value: AppModeEnum.CHAT, text: t('types.chatbot', { ns: 'app' }), icon: }, + { value: AppModeEnum.AGENT_CHAT, text: t('types.agent', { ns: 'app' }), icon: }, + { value: AppModeEnum.COMPLETION, text: t('types.completion', { ns: 'app' }), icon: }, ] useEffect(() => { @@ -140,11 +118,6 @@ const List: FC = ({ } }, [refetch]) - useEffect(() => { - if (isCurrentWorkspaceDatasetOperator) - return router.replace('/datasets') - }, [router, isCurrentWorkspaceDatasetOperator]) - useEffect(() => { if (isCurrentWorkspaceDatasetOperator) return @@ -272,7 +245,7 @@ const List: FC = ({ role="region" aria-label={t('newApp.dropDSLToCreateApp', { ns: 'app' })} > - + {t('newApp.dropDSLToCreateApp', { ns: 'app' })} )} diff --git a/web/app/components/datasets/list/__tests__/index.spec.tsx b/web/app/components/datasets/list/__tests__/index.spec.tsx index 3e6d696c5b..73e0ba0960 100644 --- a/web/app/components/datasets/list/__tests__/index.spec.tsx +++ b/web/app/components/datasets/list/__tests__/index.spec.tsx @@ -232,7 +232,7 @@ describe('List', () => { }) describe('Branch Coverage', () => { - it('should redirect normal role users to /apps', async () => { + it('should not redirect normal role users at component level', async () => { // Re-mock useAppContext with normal role vi.doMock('@/context/app-context', () => ({ useAppContext: () => ({ @@ -249,7 +249,7 @@ describe('List', () => { render() await waitFor(() => { - expect(mockReplace).toHaveBeenCalledWith('/apps') + expect(mockReplace).not.toHaveBeenCalled() }) }) diff --git a/web/app/components/datasets/list/index.tsx b/web/app/components/datasets/list/index.tsx index fdbe33986a..160186806f 100644 --- a/web/app/components/datasets/list/index.tsx +++ b/web/app/components/datasets/list/index.tsx @@ -1,9 +1,8 @@ 'use client' import { useBoolean, useDebounceFn } from 'ahooks' -import { useRouter } from 'next/navigation' // Libraries -import { useEffect, useState } from 'react' +import { useState } from 'react' import { useTranslation } from 'react-i18next' import Button from '@/app/components/base/button' @@ -28,8 +27,7 @@ import Datasets from './datasets' const List = () => { const { t } = useTranslation() const { systemFeatures } = useGlobalPublicStore() - const router = useRouter() - const { currentWorkspace, isCurrentWorkspaceOwner } = useAppContext() + const { isCurrentWorkspaceOwner } = useAppContext() const showTagManagementModal = useTagStore(s => s.showTagManagementModal) const { showExternalApiPanel, setShowExternalApiPanel } = useExternalApiPanel() const [includeAll, { toggle: toggleIncludeAll }] = useBoolean(false) @@ -54,11 +52,6 @@ const List = () => { handleTagsUpdate() } - useEffect(() => { - if (currentWorkspace.role === 'normal') - return router.replace('/apps') - }, [currentWorkspace, router]) - const isCurrentWorkspaceManager = useAppContextSelector(state => state.isCurrentWorkspaceManager) const { data: apiBaseInfo } = useDatasetApiBaseUrl() @@ -96,7 +89,7 @@ const List = () => { onClick={() => setShowExternalApiPanel(true)} > -
{t('externalAPIPanelTitle', { ns: 'dataset' })}
+
{t('externalAPIPanelTitle', { ns: 'dataset' })}
diff --git a/web/app/components/explore/__tests__/index.spec.tsx b/web/app/components/explore/__tests__/index.spec.tsx index 9f87d7afce..cf76593613 100644 --- a/web/app/components/explore/__tests__/index.spec.tsx +++ b/web/app/components/explore/__tests__/index.spec.tsx @@ -63,7 +63,7 @@ describe('Explore', () => { }) describe('Effects', () => { - it('should redirect dataset operators to /datasets', async () => { + it('should not redirect dataset operators at component level', async () => { ;(useAppContext as Mock).mockReturnValue({ isCurrentWorkspaceDatasetOperator: true, }) @@ -75,7 +75,7 @@ describe('Explore', () => { )) await waitFor(() => { - expect(mockReplace).toHaveBeenCalledWith('/datasets') + expect(mockReplace).not.toHaveBeenCalled() }) }) diff --git a/web/app/components/explore/index.tsx b/web/app/components/explore/index.tsx index 1533c6fa2a..f29ae3156e 100644 --- a/web/app/components/explore/index.tsx +++ b/web/app/components/explore/index.tsx @@ -1,23 +1,12 @@ 'use client' -import { useRouter } from 'next/navigation' import * as React from 'react' -import { useEffect } from 'react' import Sidebar from '@/app/components/explore/sidebar' -import { useAppContext } from '@/context/app-context' const Explore = ({ children, }: { children: React.ReactNode }) => { - const router = useRouter() - const { isCurrentWorkspaceDatasetOperator } = useAppContext() - - useEffect(() => { - if (isCurrentWorkspaceDatasetOperator) - router.replace('/datasets') - }, [isCurrentWorkspaceDatasetOperator, router]) - return (
diff --git a/web/eslint-suppressions.json b/web/eslint-suppressions.json index 4353273a53..c88a3550a0 100644 --- a/web/eslint-suppressions.json +++ b/web/eslint-suppressions.json @@ -1241,11 +1241,6 @@ "count": 1 } }, - "app/components/apps/list.tsx": { - "unused-imports/no-unused-vars": { - "count": 1 - } - }, "app/components/apps/new-app-card.tsx": { "ts/no-explicit-any": { "count": 1 @@ -3873,11 +3868,6 @@ "count": 1 } }, - "app/components/datasets/list/index.tsx": { - "tailwindcss/enforce-consistent-class-order": { - "count": 1 - } - }, "app/components/datasets/list/new-dataset-card/option.tsx": { "tailwindcss/enforce-consistent-class-order": { "count": 1