diff --git a/.agents/skills/frontend-testing/SKILL.md b/.agents/skills/frontend-testing/SKILL.md index 280fcb6341..69c099a262 100644 --- a/.agents/skills/frontend-testing/SKILL.md +++ b/.agents/skills/frontend-testing/SKILL.md @@ -204,6 +204,16 @@ When assigned to test a directory/path, test **ALL content** within that path: > See [Test Structure Template](#test-structure-template) for correct import/mock patterns. +### `nuqs` Query State Testing (Required for URL State Hooks) + +When a component or hook uses `useQueryState` / `useQueryStates`: + +- ✅ Use `NuqsTestingAdapter` (prefer shared helpers in `web/test/nuqs-testing.tsx`) +- ✅ Assert URL synchronization via `onUrlUpdate` (`searchParams`, `options.history`) +- ✅ For custom parsers (`createParser`), keep `parse` and `serialize` bijective and add round-trip edge cases (`%2F`, `%25`, spaces, legacy encoded values) +- ✅ Verify default-clearing behavior (default values should be removed from URL when applicable) +- ⚠️ Only mock `nuqs` directly when URL behavior is explicitly out of scope for the test + ## Core Principles ### 1. AAA Pattern (Arrange-Act-Assert) diff --git a/.agents/skills/frontend-testing/references/checklist.md b/.agents/skills/frontend-testing/references/checklist.md index 1ff2b27bbb..10b8fb66f9 100644 --- a/.agents/skills/frontend-testing/references/checklist.md +++ b/.agents/skills/frontend-testing/references/checklist.md @@ -80,6 +80,9 @@ Use this checklist when generating or reviewing tests for Dify frontend componen - [ ] Router mocks match actual Next.js API - [ ] Mocks reflect actual component conditional behavior - [ ] Only mock: API services, complex context providers, third-party libs +- [ ] For `nuqs` URL-state tests, wrap with `NuqsTestingAdapter` (prefer `web/test/nuqs-testing.tsx`) +- [ ] For `nuqs` URL-state tests, assert `onUrlUpdate` payload (`searchParams`, `options.history`) +- [ ] If custom `nuqs` parser exists, add round-trip tests for encoded edge cases (`%2F`, `%25`, spaces, legacy encoded values) ### Queries diff --git a/.agents/skills/frontend-testing/references/mocking.md b/.agents/skills/frontend-testing/references/mocking.md index 86bd375987..f58377c4a5 100644 --- a/.agents/skills/frontend-testing/references/mocking.md +++ b/.agents/skills/frontend-testing/references/mocking.md @@ -125,6 +125,31 @@ describe('Component', () => { }) ``` +### 2.1 `nuqs` Query State (Preferred: Testing Adapter) + +For tests that validate URL query behavior, use `NuqsTestingAdapter` instead of mocking `nuqs` directly. + +```typescript +import { renderHookWithNuqs } from '@/test/nuqs-testing' + +it('should sync query to URL with push history', async () => { + const { result, onUrlUpdate } = renderHookWithNuqs(() => useMyQueryState(), { + searchParams: '?page=1', + }) + + act(() => { + result.current.setQuery({ page: 2 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.options.history).toBe('push') + expect(update.searchParams.get('page')).toBe('2') +}) +``` + +Use direct `vi.mock('nuqs')` only when URL synchronization is intentionally out of scope. + ### 3. Portal Components (with Shared State) ```typescript diff --git a/web/__tests__/apps/app-list-browsing-flow.test.tsx b/web/__tests__/apps/app-list-browsing-flow.test.tsx index 1c046f5dd0..19288ecd95 100644 --- a/web/__tests__/apps/app-list-browsing-flow.test.tsx +++ b/web/__tests__/apps/app-list-browsing-flow.test.tsx @@ -8,11 +8,11 @@ */ import type { AppListResponse } from '@/models/app' import type { App } from '@/types/app' -import { fireEvent, render, screen } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { fireEvent, screen } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import List from '@/app/components/apps/list' import { AccessMode } from '@/models/access-control' +import { renderWithNuqs } from '@/test/nuqs-testing' import { AppModeEnum } from '@/types/app' let mockIsCurrentWorkspaceEditor = true @@ -161,10 +161,9 @@ const createPage = (apps: App[], hasMore = false, page = 1): AppListResponse => }) const renderList = (searchParams?: Record) => { - return render( - - - , + return renderWithNuqs( + , + { searchParams }, ) } @@ -209,11 +208,7 @@ describe('App List Browsing Flow', () => { it('should transition from loading to content when data loads', () => { mockIsLoading = true - const { rerender } = render( - - - , - ) + const { rerender } = renderWithNuqs() const skeletonCards = document.querySelectorAll('.animate-pulse') expect(skeletonCards.length).toBeGreaterThan(0) @@ -224,11 +219,7 @@ describe('App List Browsing Flow', () => { createMockApp({ id: 'app-1', name: 'Loaded App' }), ])] - rerender( - - - , - ) + rerender() expect(screen.getByText('Loaded App')).toBeInTheDocument() }) @@ -424,17 +415,9 @@ describe('App List Browsing Flow', () => { it('should call refetch when controlRefreshList increments', () => { mockPages = [createPage([createMockApp()])] - const { rerender } = render( - - - , - ) + const { rerender } = renderWithNuqs() - rerender( - - - , - ) + rerender() expect(mockRefetch).toHaveBeenCalled() }) diff --git a/web/__tests__/apps/create-app-flow.test.tsx b/web/__tests__/apps/create-app-flow.test.tsx index 556c973b06..a0976d32cc 100644 --- a/web/__tests__/apps/create-app-flow.test.tsx +++ b/web/__tests__/apps/create-app-flow.test.tsx @@ -9,11 +9,11 @@ */ import type { AppListResponse } from '@/models/app' import type { App } from '@/types/app' -import { fireEvent, render, screen, waitFor } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { fireEvent, screen, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import List from '@/app/components/apps/list' import { AccessMode } from '@/models/access-control' +import { renderWithNuqs } from '@/test/nuqs-testing' import { AppModeEnum } from '@/types/app' let mockIsCurrentWorkspaceEditor = true @@ -214,11 +214,7 @@ const createPage = (apps: App[]): AppListResponse => ({ }) const renderList = () => { - return render( - - - , - ) + return renderWithNuqs() } describe('Create App Flow', () => { diff --git a/web/__tests__/datasets/document-management.test.tsx b/web/__tests__/datasets/document-management.test.tsx index 3b901ccee2..8aedd4fc63 100644 --- a/web/__tests__/datasets/document-management.test.tsx +++ b/web/__tests__/datasets/document-management.test.tsx @@ -7,9 +7,10 @@ */ import type { SimpleDocumentDetail } from '@/models/datasets' -import { act, renderHook } from '@testing-library/react' +import { act, renderHook, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { DataSourceType } from '@/models/datasets' +import { renderHookWithNuqs } from '@/test/nuqs-testing' const mockPush = vi.fn() vi.mock('next/navigation', () => ({ @@ -28,12 +29,16 @@ const { useDocumentSort } = await import( const { useDocumentSelection } = await import( '@/app/components/datasets/documents/components/document-list/hooks/use-document-selection', ) -const { default: useDocumentListQueryState } = await import( +const { useDocumentListQueryState } = await import( '@/app/components/datasets/documents/hooks/use-document-list-query-state', ) type LocalDoc = SimpleDocumentDetail & { percent?: number } +const renderQueryStateHook = (searchParams = '') => { + return renderHookWithNuqs(() => useDocumentListQueryState(), { searchParams }) +} + const createDoc = (overrides?: Partial): LocalDoc => ({ id: `doc-${Math.random().toString(36).slice(2, 8)}`, name: 'test-doc.txt', @@ -85,7 +90,7 @@ describe('Document Management Flow', () => { describe('URL-based Query State', () => { it('should parse default query from empty URL params', () => { - const { result } = renderHook(() => useDocumentListQueryState()) + const { result } = renderQueryStateHook() expect(result.current.query).toEqual({ page: 1, @@ -96,107 +101,85 @@ describe('Document Management Flow', () => { }) }) - it('should update query and push to router', () => { - const { result } = renderHook(() => useDocumentListQueryState()) + it('should update keyword query with replace history', async () => { + const { result, onUrlUpdate } = renderQueryStateHook() act(() => { result.current.updateQuery({ keyword: 'test', page: 2 }) }) - expect(mockPush).toHaveBeenCalled() - // The push call should contain the updated query params - const pushUrl = mockPush.mock.calls[0][0] as string - expect(pushUrl).toContain('keyword=test') - expect(pushUrl).toContain('page=2') + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.options.history).toBe('replace') + expect(update.searchParams.get('keyword')).toBe('test') + expect(update.searchParams.get('page')).toBe('2') }) - it('should reset query to defaults', () => { - const { result } = renderHook(() => useDocumentListQueryState()) + it('should reset query to defaults', async () => { + const { result, onUrlUpdate } = renderQueryStateHook() act(() => { result.current.resetQuery() }) - expect(mockPush).toHaveBeenCalled() - // Default query omits default values from URL - const pushUrl = mockPush.mock.calls[0][0] as string - expect(pushUrl).toBe('/datasets/ds-1/documents') + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.options.history).toBe('replace') + expect(update.searchParams.toString()).toBe('') }) }) describe('Document Sort Integration', () => { - it('should return documents unsorted when no sort field set', () => { - const docs = [ - createDoc({ id: 'doc-1', name: 'Banana.txt', word_count: 300 }), - createDoc({ id: 'doc-2', name: 'Apple.txt', word_count: 100 }), - createDoc({ id: 'doc-3', name: 'Cherry.txt', word_count: 200 }), - ] - + it('should derive sort field and order from remote sort value', () => { const { result } = renderHook(() => useDocumentSort({ - documents: docs, - statusFilterValue: '', remoteSortValue: '-created_at', + onRemoteSortChange: vi.fn(), })) - expect(result.current.sortField).toBeNull() - expect(result.current.sortedDocuments).toHaveLength(3) + expect(result.current.sortField).toBe('created_at') + expect(result.current.sortOrder).toBe('desc') }) - it('should sort by name descending', () => { - const docs = [ - createDoc({ id: 'doc-1', name: 'Banana.txt' }), - createDoc({ id: 'doc-2', name: 'Apple.txt' }), - createDoc({ id: 'doc-3', name: 'Cherry.txt' }), - ] - + it('should call remote sort change with descending sort for a new field', () => { + const onRemoteSortChange = vi.fn() const { result } = renderHook(() => useDocumentSort({ - documents: docs, - statusFilterValue: '', remoteSortValue: '-created_at', + onRemoteSortChange, })) act(() => { - result.current.handleSort('name') + result.current.handleSort('hit_count') }) - expect(result.current.sortField).toBe('name') - expect(result.current.sortOrder).toBe('desc') - const names = result.current.sortedDocuments.map(d => d.name) - expect(names).toEqual(['Cherry.txt', 'Banana.txt', 'Apple.txt']) + expect(onRemoteSortChange).toHaveBeenCalledWith('-hit_count') }) - it('should toggle sort order on same field click', () => { - const docs = [createDoc({ id: 'doc-1', name: 'A.txt' }), createDoc({ id: 'doc-2', name: 'B.txt' })] - + it('should toggle descending to ascending when clicking active field', () => { + const onRemoteSortChange = vi.fn() const { result } = renderHook(() => useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '-created_at', + remoteSortValue: '-hit_count', + onRemoteSortChange, })) - act(() => result.current.handleSort('name')) - expect(result.current.sortOrder).toBe('desc') + act(() => { + result.current.handleSort('hit_count') + }) - act(() => result.current.handleSort('name')) - expect(result.current.sortOrder).toBe('asc') + expect(onRemoteSortChange).toHaveBeenCalledWith('hit_count') }) - it('should filter by status before sorting', () => { - const docs = [ - createDoc({ id: 'doc-1', name: 'A.txt', display_status: 'available' }), - createDoc({ id: 'doc-2', name: 'B.txt', display_status: 'error' }), - createDoc({ id: 'doc-3', name: 'C.txt', display_status: 'available' }), - ] - + it('should ignore null sort field updates', () => { + const onRemoteSortChange = vi.fn() const { result } = renderHook(() => useDocumentSort({ - documents: docs, - statusFilterValue: 'available', remoteSortValue: '-created_at', + onRemoteSortChange, })) - // Only 'available' documents should remain - expect(result.current.sortedDocuments).toHaveLength(2) - expect(result.current.sortedDocuments.every(d => d.display_status === 'available')).toBe(true) + act(() => { + result.current.handleSort(null) + }) + + expect(onRemoteSortChange).not.toHaveBeenCalled() }) }) @@ -309,14 +292,13 @@ describe('Document Management Flow', () => { describe('Cross-Module: Query State → Sort → Selection Pipeline', () => { it('should maintain consistent default state across all hooks', () => { const docs = [createDoc({ id: 'doc-1' })] - const { result: queryResult } = renderHook(() => useDocumentListQueryState()) + const { result: queryResult } = renderQueryStateHook() const { result: sortResult } = renderHook(() => useDocumentSort({ - documents: docs, - statusFilterValue: queryResult.current.query.status, remoteSortValue: queryResult.current.query.sort, + onRemoteSortChange: vi.fn(), })) const { result: selResult } = renderHook(() => useDocumentSelection({ - documents: sortResult.current.sortedDocuments, + documents: docs, selectedIds: [], onSelectedIdChange: vi.fn(), })) @@ -325,8 +307,9 @@ describe('Document Management Flow', () => { expect(queryResult.current.query.sort).toBe('-created_at') expect(queryResult.current.query.status).toBe('all') - // Sort inherits 'all' status → no filtering applied - expect(sortResult.current.sortedDocuments).toHaveLength(1) + // Sort state is derived from URL default sort. + expect(sortResult.current.sortField).toBe('created_at') + expect(sortResult.current.sortOrder).toBe('desc') // Selection starts empty expect(selResult.current.isAllSelected).toBe(false) diff --git a/web/__tests__/tools/tool-browsing-and-filtering.test.tsx b/web/__tests__/tools/tool-browsing-and-filtering.test.tsx index 4e7fa4952b..dbefb1fdc3 100644 --- a/web/__tests__/tools/tool-browsing-and-filtering.test.tsx +++ b/web/__tests__/tools/tool-browsing-and-filtering.test.tsx @@ -28,9 +28,13 @@ vi.mock('react-i18next', () => ({ }), })) -vi.mock('nuqs', () => ({ - useQueryState: () => ['builtin', vi.fn()], -})) +vi.mock('nuqs', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useQueryState: () => ['builtin', vi.fn()], + } +}) vi.mock('@/context/global-public-context', () => ({ useGlobalPublicStore: () => ({ enable_marketplace: false }), @@ -212,6 +216,12 @@ vi.mock('@/app/components/tools/marketplace', () => ({ default: () => null, })) +vi.mock('@/app/components/tools/marketplace/hooks', () => ({ + useMarketplace: () => ({ + handleScroll: vi.fn(), + }), +})) + vi.mock('@/app/components/tools/mcp', () => ({ default: () =>
MCP List
, })) diff --git a/web/app/components/apps/__tests__/list.spec.tsx b/web/app/components/apps/__tests__/list.spec.tsx index fa83296267..a9bef08243 100644 --- a/web/app/components/apps/__tests__/list.spec.tsx +++ b/web/app/components/apps/__tests__/list.spec.tsx @@ -1,9 +1,7 @@ -import type { UrlUpdateEvent } from 'nuqs/adapters/testing' -import type { ReactNode } from 'react' -import { act, fireEvent, render, screen } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { act, fireEvent, screen } from '@testing-library/react' import * as React from 'react' import { useStore as useTagStore } from '@/app/components/base/tag-management/store' +import { renderWithNuqs } from '@/test/nuqs-testing' import { AppModeEnum } from '@/types/app' import List from '../list' @@ -186,21 +184,14 @@ beforeAll(() => { } as unknown as typeof IntersectionObserver }) -// Render helper wrapping with NuqsTestingAdapter -const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>() +// Render helper wrapping with shared nuqs testing helper. const renderList = (searchParams = '') => { - const wrapper = ({ children }: { children: ReactNode }) => ( - - {children} - - ) - return render(, { wrapper }) + return renderWithNuqs(, { searchParams }) } describe('List', () => { beforeEach(() => { vi.clearAllMocks() - onUrlUpdate.mockClear() useTagStore.setState({ tagList: [{ id: 'tag-1', name: 'Test Tag', type: 'app', binding_count: 0 }], showTagManagementModal: false, @@ -277,7 +268,7 @@ describe('List', () => { describe('Tab Navigation', () => { it('should update URL when workflow tab is clicked', async () => { - renderList() + const { onUrlUpdate } = renderList() fireEvent.click(screen.getByText('app.types.workflow')) @@ -287,7 +278,7 @@ describe('List', () => { }) it('should update URL when all tab is clicked', async () => { - renderList('?category=workflow') + const { onUrlUpdate } = renderList('?category=workflow') fireEvent.click(screen.getByText('app.types.all')) @@ -391,18 +382,10 @@ describe('List', () => { describe('Edge Cases', () => { it('should handle multiple renders without issues', () => { - const { rerender } = render( - - - , - ) + const { rerender } = renderWithNuqs() expect(screen.getByText('app.types.all')).toBeInTheDocument() - rerender( - - - , - ) + rerender() expect(screen.getByText('app.types.all')).toBeInTheDocument() }) @@ -448,7 +431,7 @@ describe('List', () => { }) it('should update URL for each app type tab click', async () => { - renderList() + const { onUrlUpdate } = renderList() const appTypeTexts = [ { mode: AppModeEnum.WORKFLOW, text: 'app.types.workflow' }, diff --git a/web/app/components/apps/hooks/__tests__/use-apps-query-state.spec.tsx b/web/app/components/apps/hooks/__tests__/use-apps-query-state.spec.tsx index 0c956b78a4..8e8e5821a8 100644 --- a/web/app/components/apps/hooks/__tests__/use-apps-query-state.spec.tsx +++ b/web/app/components/apps/hooks/__tests__/use-apps-query-state.spec.tsx @@ -1,18 +1,9 @@ -import type { UrlUpdateEvent } from 'nuqs/adapters/testing' -import type { ReactNode } from 'react' -import { act, renderHook, waitFor } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { act, waitFor } from '@testing-library/react' +import { renderHookWithNuqs } from '@/test/nuqs-testing' import useAppsQueryState from '../use-apps-query-state' const renderWithAdapter = (searchParams = '') => { - const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>() - const wrapper = ({ children }: { children: ReactNode }) => ( - - {children} - - ) - const { result } = renderHook(() => useAppsQueryState(), { wrapper }) - return { result, onUrlUpdate } + return renderHookWithNuqs(() => useAppsQueryState(), { searchParams }) } describe('useAppsQueryState', () => { diff --git a/web/app/components/apps/list.tsx b/web/app/components/apps/list.tsx index d97cd176ca..6ae422f716 100644 --- a/web/app/components/apps/list.tsx +++ b/web/app/components/apps/list.tsx @@ -3,7 +3,7 @@ import type { FC } from 'react' import { useDebounceFn } from 'ahooks' import dynamic from 'next/dynamic' -import { parseAsString, useQueryState } from 'nuqs' +import { parseAsStringLiteral, useQueryState } from 'nuqs' import { useCallback, useEffect, useRef, useState } from 'react' import { useTranslation } from 'react-i18next' import Input from '@/app/components/base/input' @@ -16,7 +16,7 @@ import { useAppContext } from '@/context/app-context' import { useGlobalPublicStore } from '@/context/global-public-context' import { CheckModal } from '@/hooks/use-pay' import { useInfiniteAppList } from '@/service/use-apps' -import { AppModeEnum } from '@/types/app' +import { AppModeEnum, AppModes } from '@/types/app' import { cn } from '@/utils/classnames' import AppCard from './app-card' import { AppCardSkeleton } from './app-card-skeleton' @@ -33,6 +33,18 @@ const CreateFromDSLModal = dynamic(() => import('@/app/components/app/create-fro ssr: false, }) +const APP_LIST_CATEGORY_VALUES = ['all', ...AppModes] as const +type AppListCategory = typeof APP_LIST_CATEGORY_VALUES[number] +const appListCategorySet = new Set(APP_LIST_CATEGORY_VALUES) + +const isAppListCategory = (value: string): value is AppListCategory => { + return appListCategorySet.has(value) +} + +const parseAsAppListCategory = parseAsStringLiteral(APP_LIST_CATEGORY_VALUES) + .withDefault('all') + .withOptions({ history: 'push' }) + type Props = { controlRefreshList?: number } @@ -45,7 +57,7 @@ const List: FC = ({ const showTagManagementModal = useTagStore(s => s.showTagManagementModal) const [activeTab, setActiveTab] = useQueryState( 'category', - parseAsString.withDefault('all').withOptions({ history: 'push' }), + parseAsAppListCategory, ) const { query: { tagIDs = [], keywords = '', isCreatedByMe: queryIsCreatedByMe = false }, setQuery } = useAppsQueryState() @@ -80,7 +92,7 @@ const List: FC = ({ name: searchKeywords, tag_ids: tagIDs, is_created_by_me: isCreatedByMe, - ...(activeTab !== 'all' ? { mode: activeTab as AppModeEnum } : {}), + ...(activeTab !== 'all' ? { mode: activeTab } : {}), } const { @@ -186,7 +198,10 @@ const List: FC = ({
{ + if (isAppListCategory(nextValue)) + setActiveTab(nextValue) + }} options={options} />
diff --git a/web/app/components/datasets/documents/__tests__/index.spec.tsx b/web/app/components/datasets/documents/__tests__/index.spec.tsx index 1749508ee1..f464c97395 100644 --- a/web/app/components/datasets/documents/__tests__/index.spec.tsx +++ b/web/app/components/datasets/documents/__tests__/index.spec.tsx @@ -4,7 +4,7 @@ import { useDatasetDetailContextWithSelector } from '@/context/dataset-detail' import { useProviderContext } from '@/context/provider-context' import { DataSourceType } from '@/models/datasets' import { useDocumentList } from '@/service/knowledge/use-document' -import useDocumentsPageState from '../hooks/use-documents-page-state' +import { useDocumentsPageState } from '../hooks/use-documents-page-state' import Documents from '../index' // Type for mock selector function - use `as MockState` to bypass strict type checking in tests @@ -117,13 +117,10 @@ const mockHandleStatusFilterClear = vi.fn() const mockHandleSortChange = vi.fn() const mockHandlePageChange = vi.fn() const mockHandleLimitChange = vi.fn() -const mockUpdatePollingState = vi.fn() -const mockAdjustPageForTotal = vi.fn() vi.mock('../hooks/use-documents-page-state', () => ({ - default: vi.fn(() => ({ + useDocumentsPageState: vi.fn(() => ({ inputValue: '', - searchValue: '', debouncedSearchValue: '', handleInputChange: mockHandleInputChange, statusFilterValue: 'all', @@ -138,9 +135,6 @@ vi.mock('../hooks/use-documents-page-state', () => ({ handleLimitChange: mockHandleLimitChange, selectedIds: [] as string[], setSelectedIds: mockSetSelectedIds, - timerCanRun: false, - updatePollingState: mockUpdatePollingState, - adjustPageForTotal: mockAdjustPageForTotal, })), })) @@ -319,6 +313,33 @@ describe('Documents', () => { expect(screen.queryByTestId('documents-list')).not.toBeInTheDocument() }) + it('should keep rendering list when loading with existing data', () => { + vi.mocked(useDocumentList).mockReturnValueOnce({ + data: { + data: [ + { + id: 'doc-1', + name: 'Document 1', + indexing_status: 'completed', + data_source_type: 'upload_file', + position: 1, + enabled: true, + }, + ], + total: 1, + page: 1, + limit: 10, + has_more: false, + } as DocumentListResponse, + isLoading: true, + refetch: vi.fn(), + } as unknown as ReturnType) + + render() + expect(screen.getByTestId('documents-list')).toBeInTheDocument() + expect(screen.getByTestId('list-documents-count')).toHaveTextContent('1') + }) + it('should render empty element when no documents exist', () => { vi.mocked(useDocumentList).mockReturnValueOnce({ data: { data: [], total: 0, page: 1, limit: 10, has_more: false }, @@ -484,17 +505,75 @@ describe('Documents', () => { }) }) - describe('Side Effects and Cleanup', () => { - it('should call updatePollingState when documents response changes', () => { + describe('Query Options', () => { + it('should pass function refetchInterval to useDocumentList', () => { render() - expect(mockUpdatePollingState).toHaveBeenCalled() + const payload = vi.mocked(useDocumentList).mock.calls.at(-1)?.[0] + expect(payload).toBeDefined() + expect(typeof payload?.refetchInterval).toBe('function') }) - it('should call adjustPageForTotal when documents response changes', () => { + it('should stop polling when all documents are in terminal statuses', () => { render() - expect(mockAdjustPageForTotal).toHaveBeenCalled() + const payload = vi.mocked(useDocumentList).mock.calls.at(-1)?.[0] + const refetchInterval = payload?.refetchInterval + expect(typeof refetchInterval).toBe('function') + if (typeof refetchInterval !== 'function') + throw new Error('Expected function refetchInterval') + + const interval = refetchInterval({ + state: { + data: { + data: [ + { indexing_status: 'completed' }, + { indexing_status: 'paused' }, + { indexing_status: 'error' }, + ], + }, + }, + } as unknown as Parameters[0]) + + expect(interval).toBe(false) + }) + + it('should keep polling for transient status filters', () => { + vi.mocked(useDocumentsPageState).mockReturnValueOnce({ + inputValue: '', + debouncedSearchValue: '', + handleInputChange: mockHandleInputChange, + statusFilterValue: 'indexing', + sortValue: '-created_at' as const, + normalizedStatusFilterValue: 'indexing', + handleStatusFilterChange: mockHandleStatusFilterChange, + handleStatusFilterClear: mockHandleStatusFilterClear, + handleSortChange: mockHandleSortChange, + currPage: 0, + limit: 10, + handlePageChange: mockHandlePageChange, + handleLimitChange: mockHandleLimitChange, + selectedIds: [] as string[], + setSelectedIds: mockSetSelectedIds, + }) + + render() + + const payload = vi.mocked(useDocumentList).mock.calls.at(-1)?.[0] + const refetchInterval = payload?.refetchInterval + expect(typeof refetchInterval).toBe('function') + if (typeof refetchInterval !== 'function') + throw new Error('Expected function refetchInterval') + + const interval = refetchInterval({ + state: { + data: { + data: [{ indexing_status: 'completed' }], + }, + }, + } as unknown as Parameters[0]) + + expect(interval).toBe(2500) }) }) @@ -591,36 +670,6 @@ describe('Documents', () => { }) }) - describe('Polling State', () => { - it('should enable polling when documents are indexing', () => { - vi.mocked(useDocumentsPageState).mockReturnValueOnce({ - inputValue: '', - searchValue: '', - debouncedSearchValue: '', - handleInputChange: mockHandleInputChange, - statusFilterValue: 'all', - sortValue: '-created_at' as const, - normalizedStatusFilterValue: 'all', - handleStatusFilterChange: mockHandleStatusFilterChange, - handleStatusFilterClear: mockHandleStatusFilterClear, - handleSortChange: mockHandleSortChange, - currPage: 0, - limit: 10, - handlePageChange: mockHandlePageChange, - handleLimitChange: mockHandleLimitChange, - selectedIds: [] as string[], - setSelectedIds: mockSetSelectedIds, - timerCanRun: true, - updatePollingState: mockUpdatePollingState, - adjustPageForTotal: mockAdjustPageForTotal, - }) - - render() - - expect(screen.getByTestId('documents-list')).toBeInTheDocument() - }) - }) - describe('Pagination', () => { it('should display correct total in list', () => { render() @@ -635,7 +684,6 @@ describe('Documents', () => { it('should handle page changes', () => { vi.mocked(useDocumentsPageState).mockReturnValueOnce({ inputValue: '', - searchValue: '', debouncedSearchValue: '', handleInputChange: mockHandleInputChange, statusFilterValue: 'all', @@ -650,9 +698,6 @@ describe('Documents', () => { handleLimitChange: mockHandleLimitChange, selectedIds: [] as string[], setSelectedIds: mockSetSelectedIds, - timerCanRun: false, - updatePollingState: mockUpdatePollingState, - adjustPageForTotal: mockAdjustPageForTotal, }) render() @@ -664,7 +709,6 @@ describe('Documents', () => { it('should display selected count', () => { vi.mocked(useDocumentsPageState).mockReturnValueOnce({ inputValue: '', - searchValue: '', debouncedSearchValue: '', handleInputChange: mockHandleInputChange, statusFilterValue: 'all', @@ -679,9 +723,6 @@ describe('Documents', () => { handleLimitChange: mockHandleLimitChange, selectedIds: ['doc-1', 'doc-2'], setSelectedIds: mockSetSelectedIds, - timerCanRun: false, - updatePollingState: mockUpdatePollingState, - adjustPageForTotal: mockAdjustPageForTotal, }) render() @@ -693,7 +734,6 @@ describe('Documents', () => { it('should pass filter value to list', () => { vi.mocked(useDocumentsPageState).mockReturnValueOnce({ inputValue: 'test search', - searchValue: 'test search', debouncedSearchValue: 'test search', handleInputChange: mockHandleInputChange, statusFilterValue: 'completed', @@ -708,9 +748,6 @@ describe('Documents', () => { handleLimitChange: mockHandleLimitChange, selectedIds: [] as string[], setSelectedIds: mockSetSelectedIds, - timerCanRun: false, - updatePollingState: mockUpdatePollingState, - adjustPageForTotal: mockAdjustPageForTotal, }) render() diff --git a/web/app/components/datasets/documents/components/__tests__/list.spec.tsx b/web/app/components/datasets/documents/components/__tests__/list.spec.tsx index a96afe3cb4..bb7e170783 100644 --- a/web/app/components/datasets/documents/components/__tests__/list.spec.tsx +++ b/web/app/components/datasets/documents/components/__tests__/list.spec.tsx @@ -20,9 +20,8 @@ const mockHandleSave = vi.fn() vi.mock('../document-list/hooks', () => ({ useDocumentSort: vi.fn(() => ({ sortField: null, - sortOrder: null, + sortOrder: 'desc', handleSort: mockHandleSort, - sortedDocuments: [], })), useDocumentSelection: vi.fn(() => ({ isAllSelected: false, @@ -125,8 +124,8 @@ const defaultProps = { pagination: { total: 0, current: 1, limit: 10, onChange: vi.fn() }, onUpdate: vi.fn(), onManageMetadata: vi.fn(), - statusFilterValue: 'all', - remoteSortValue: '', + remoteSortValue: '-created_at', + onSortChange: vi.fn(), } describe('DocumentList', () => { @@ -140,8 +139,6 @@ describe('DocumentList', () => { render() expect(screen.getByText('#')).toBeInTheDocument() - expect(screen.getByTestId('sort-name')).toBeInTheDocument() - expect(screen.getByTestId('sort-word_count')).toBeInTheDocument() expect(screen.getByTestId('sort-hit_count')).toBeInTheDocument() expect(screen.getByTestId('sort-created_at')).toBeInTheDocument() }) @@ -164,10 +161,9 @@ describe('DocumentList', () => { it('should render document rows from sortedDocuments', () => { const docs = [createDoc({ id: 'a', name: 'Doc A' }), createDoc({ id: 'b', name: 'Doc B' })] vi.mocked(useDocumentSort).mockReturnValue({ - sortField: null, + sortField: 'created_at', sortOrder: 'desc', handleSort: mockHandleSort, - sortedDocuments: docs, } as unknown as ReturnType) render() @@ -182,9 +178,9 @@ describe('DocumentList', () => { it('should call handleSort when sort header is clicked', () => { render() - fireEvent.click(screen.getByTestId('sort-name')) + fireEvent.click(screen.getByTestId('sort-created_at')) - expect(mockHandleSort).toHaveBeenCalledWith('name') + expect(mockHandleSort).toHaveBeenCalledWith('created_at') }) }) @@ -229,7 +225,6 @@ describe('DocumentList', () => { sortField: null, sortOrder: 'desc', handleSort: mockHandleSort, - sortedDocuments: [], } as unknown as ReturnType) render() diff --git a/web/app/components/datasets/documents/components/document-list/__tests__/index.spec.tsx b/web/app/components/datasets/documents/components/document-list/__tests__/index.spec.tsx index 5ea2a00a7d..5053038d5e 100644 --- a/web/app/components/datasets/documents/components/document-list/__tests__/index.spec.tsx +++ b/web/app/components/datasets/documents/components/document-list/__tests__/index.spec.tsx @@ -2,7 +2,7 @@ import type { ReactNode } from 'react' import type { Props as PaginationProps } from '@/app/components/base/pagination' import type { SimpleDocumentDetail } from '@/models/datasets' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' -import { fireEvent, render, screen } from '@testing-library/react' +import { act, fireEvent, render, screen } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { ChunkingMode, DataSourceType } from '@/models/datasets' import DocumentList from '../../list' @@ -13,6 +13,7 @@ vi.mock('next/navigation', () => ({ useRouter: () => ({ push: mockPush, }), + useSearchParams: () => new URLSearchParams(), })) vi.mock('@/context/dataset-detail', () => ({ @@ -90,8 +91,8 @@ describe('DocumentList', () => { pagination: defaultPagination, onUpdate: vi.fn(), onManageMetadata: vi.fn(), - statusFilterValue: '', - remoteSortValue: '', + remoteSortValue: '-created_at', + onSortChange: vi.fn(), } beforeEach(() => { @@ -220,16 +221,15 @@ describe('DocumentList', () => { expect(sortIcons.length).toBeGreaterThan(0) }) - it('should update sort order when sort header is clicked', () => { - render(, { wrapper: createWrapper() }) + it('should call onSortChange when sortable header is clicked', () => { + const onSortChange = vi.fn() + const { container } = render(, { wrapper: createWrapper() }) - // Find and click a sort header by its parent div containing the label text - const sortableHeaders = document.querySelectorAll('[class*="cursor-pointer"]') - if (sortableHeaders.length > 0) { + const sortableHeaders = container.querySelectorAll('thead button') + if (sortableHeaders.length > 0) fireEvent.click(sortableHeaders[0]) - } - expect(screen.getByRole('table')).toBeInTheDocument() + expect(onSortChange).toHaveBeenCalled() }) }) @@ -360,13 +360,15 @@ describe('DocumentList', () => { expect(modal).not.toBeInTheDocument() }) - it('should show rename modal when rename button is clicked', () => { + it('should show rename modal when rename button is clicked', async () => { const { container } = render(, { wrapper: createWrapper() }) // Find and click the rename button in the first row const renameButtons = container.querySelectorAll('.cursor-pointer.rounded-md') if (renameButtons.length > 0) { - fireEvent.click(renameButtons[0]) + await act(async () => { + fireEvent.click(renameButtons[0]) + }) } // After clicking rename, the modal should potentially be visible @@ -384,7 +386,7 @@ describe('DocumentList', () => { }) describe('Edit Metadata Modal', () => { - it('should handle edit metadata action', () => { + it('should handle edit metadata action', async () => { const props = { ...defaultProps, selectedIds: ['doc-1'], @@ -393,7 +395,9 @@ describe('DocumentList', () => { const editButton = screen.queryByRole('button', { name: /metadata/i }) if (editButton) { - fireEvent.click(editButton) + await act(async () => { + fireEvent.click(editButton) + }) } expect(screen.getByRole('table')).toBeInTheDocument() @@ -454,16 +458,6 @@ describe('DocumentList', () => { expect(screen.getByRole('table')).toBeInTheDocument() }) - it('should handle status filter value', () => { - const props = { - ...defaultProps, - statusFilterValue: 'completed', - } - render(, { wrapper: createWrapper() }) - - expect(screen.getByRole('table')).toBeInTheDocument() - }) - it('should handle remote sort value', () => { const props = { ...defaultProps, diff --git a/web/app/components/datasets/documents/components/document-list/components/__tests__/document-table-row.spec.tsx b/web/app/components/datasets/documents/components/document-list/components/__tests__/document-table-row.spec.tsx index ad920e9a37..20a3f7cee1 100644 --- a/web/app/components/datasets/documents/components/document-list/components/__tests__/document-table-row.spec.tsx +++ b/web/app/components/datasets/documents/components/document-list/components/__tests__/document-table-row.spec.tsx @@ -7,11 +7,13 @@ import { DataSourceType } from '@/models/datasets' import DocumentTableRow from '../document-table-row' const mockPush = vi.fn() +let mockSearchParams = '' vi.mock('next/navigation', () => ({ useRouter: () => ({ push: mockPush, }), + useSearchParams: () => new URLSearchParams(mockSearchParams), })) const createTestQueryClient = () => new QueryClient({ @@ -95,6 +97,7 @@ describe('DocumentTableRow', () => { beforeEach(() => { vi.clearAllMocks() + mockSearchParams = '' }) describe('Rendering', () => { @@ -186,6 +189,15 @@ describe('DocumentTableRow', () => { expect(mockPush).toHaveBeenCalledWith('/datasets/custom-dataset/documents/custom-doc') }) + + it('should preserve search params when navigating to detail', () => { + mockSearchParams = 'page=2&status=error' + render(, { wrapper: createWrapper() }) + + fireEvent.click(screen.getByRole('row')) + + expect(mockPush).toHaveBeenCalledWith('/datasets/dataset-1/documents/doc-1?page=2&status=error') + }) }) describe('Word Count Display', () => { diff --git a/web/app/components/datasets/documents/components/document-list/components/__tests__/sort-header.spec.tsx b/web/app/components/datasets/documents/components/document-list/components/__tests__/sort-header.spec.tsx index 777f240d00..8730f3f278 100644 --- a/web/app/components/datasets/documents/components/document-list/components/__tests__/sort-header.spec.tsx +++ b/web/app/components/datasets/documents/components/document-list/components/__tests__/sort-header.spec.tsx @@ -4,8 +4,8 @@ import SortHeader from '../sort-header' describe('SortHeader', () => { const defaultProps = { - field: 'name' as const, - label: 'File Name', + field: 'created_at' as const, + label: 'Upload Time', currentSortField: null, sortOrder: 'desc' as const, onSort: vi.fn(), @@ -14,12 +14,12 @@ describe('SortHeader', () => { describe('rendering', () => { it('should render the label', () => { render() - expect(screen.getByText('File Name')).toBeInTheDocument() + expect(screen.getByText('Upload Time')).toBeInTheDocument() }) it('should render the sort icon', () => { const { container } = render() - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).toBeInTheDocument() }) }) @@ -27,13 +27,13 @@ describe('SortHeader', () => { describe('inactive state', () => { it('should have disabled text color when not active', () => { const { container } = render() - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).toHaveClass('text-text-disabled') }) it('should not be rotated when not active', () => { const { container } = render() - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).not.toHaveClass('rotate-180') }) }) @@ -41,25 +41,25 @@ describe('SortHeader', () => { describe('active state', () => { it('should have tertiary text color when active', () => { const { container } = render( - , + , ) - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).toHaveClass('text-text-tertiary') }) it('should not be rotated when active and desc', () => { const { container } = render( - , + , ) - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).not.toHaveClass('rotate-180') }) it('should be rotated when active and asc', () => { const { container } = render( - , + , ) - const icon = container.querySelector('svg') + const icon = container.querySelector('button span') expect(icon).toHaveClass('rotate-180') }) }) @@ -69,34 +69,22 @@ describe('SortHeader', () => { const onSort = vi.fn() render() - fireEvent.click(screen.getByText('File Name')) + fireEvent.click(screen.getByText('Upload Time')) - expect(onSort).toHaveBeenCalledWith('name') + expect(onSort).toHaveBeenCalledWith('created_at') }) it('should call onSort with correct field', () => { const onSort = vi.fn() - render() + render() - fireEvent.click(screen.getByText('File Name')) + fireEvent.click(screen.getByText('Upload Time')) - expect(onSort).toHaveBeenCalledWith('word_count') + expect(onSort).toHaveBeenCalledWith('hit_count') }) }) describe('different fields', () => { - it('should work with word_count field', () => { - render( - , - ) - expect(screen.getByText('Words')).toBeInTheDocument() - }) - it('should work with hit_count field', () => { render( = React.memo(({ const { t } = useTranslation() const { formatTime } = useTimestamp() const router = useRouter() + const searchParams = useSearchParams() const isFile = doc.data_source_type === DataSourceType.FILE const fileType = isFile ? doc.data_source_detail_dict?.upload_file?.extension : '' + const queryString = searchParams.toString() const handleRowClick = useCallback(() => { - router.push(`/datasets/${datasetId}/documents/${doc.id}`) - }, [router, datasetId, doc.id]) + router.push(`/datasets/${datasetId}/documents/${doc.id}${queryString ? `?${queryString}` : ''}`) + }, [router, datasetId, doc.id, queryString]) const handleCheckboxClick = useCallback((e: React.MouseEvent) => { e.stopPropagation() @@ -100,7 +101,7 @@ const DocumentTableRow: FC = React.memo(({
- {doc.name} + {doc.name} {doc.summary_index_status && (
@@ -113,7 +114,7 @@ const DocumentTableRow: FC = React.memo(({ className="cursor-pointer rounded-md p-1 hover:bg-state-base-hover" onClick={handleRenameClick} > - +
diff --git a/web/app/components/datasets/documents/components/document-list/components/sort-header.tsx b/web/app/components/datasets/documents/components/document-list/components/sort-header.tsx index 1dc13df2b0..1d693565cb 100644 --- a/web/app/components/datasets/documents/components/document-list/components/sort-header.tsx +++ b/web/app/components/datasets/documents/components/document-list/components/sort-header.tsx @@ -1,6 +1,5 @@ import type { FC } from 'react' import type { SortField, SortOrder } from '../hooks' -import { RiArrowDownLine } from '@remixicon/react' import * as React from 'react' import { cn } from '@/utils/classnames' @@ -23,19 +22,20 @@ const SortHeader: FC = React.memo(({ const isDesc = isActive && sortOrder === 'desc' return ( -
onSort(field)} > {label} - -
+ ) }) diff --git a/web/app/components/datasets/documents/components/document-list/hooks/__tests__/use-document-sort.spec.ts b/web/app/components/datasets/documents/components/document-list/hooks/__tests__/use-document-sort.spec.ts index 43bc0e1dd5..004597afa9 100644 --- a/web/app/components/datasets/documents/components/document-list/hooks/__tests__/use-document-sort.spec.ts +++ b/web/app/components/datasets/documents/components/document-list/hooks/__tests__/use-document-sort.spec.ts @@ -1,340 +1,98 @@ -import type { SimpleDocumentDetail } from '@/models/datasets' import { act, renderHook } from '@testing-library/react' -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { useDocumentSort } from '../use-document-sort' -type LocalDoc = SimpleDocumentDetail & { percent?: number } - -const createMockDocument = (overrides: Partial = {}): LocalDoc => ({ - id: 'doc1', - name: 'Test Document', - data_source_type: 'upload_file', - data_source_info: {}, - data_source_detail_dict: {}, - word_count: 100, - hit_count: 10, - created_at: 1000000, - position: 1, - doc_form: 'text_model', - enabled: true, - archived: false, - display_status: 'available', - created_from: 'api', - ...overrides, -} as LocalDoc) - describe('useDocumentSort', () => { - describe('initial state', () => { - it('should return null sortField initially', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) + describe('remote state parsing', () => { + it('should parse descending created_at sort', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: '-created_at', + onRemoteSortChange, + })) - expect(result.current.sortField).toBeNull() + expect(result.current.sortField).toBe('created_at') expect(result.current.sortOrder).toBe('desc') }) - it('should return documents unchanged when no sort is applied', () => { - const docs = [ - createMockDocument({ id: 'doc1', name: 'B' }), - createMockDocument({ id: 'doc2', name: 'A' }), - ] + it('should parse ascending hit_count sort', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: 'hit_count', + onRemoteSortChange, + })) - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) + expect(result.current.sortField).toBe('hit_count') + expect(result.current.sortOrder).toBe('asc') + }) - expect(result.current.sortedDocuments).toEqual(docs) + it('should fallback to inactive field for unsupported sort key', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: '-name', + onRemoteSortChange, + })) + + expect(result.current.sortField).toBeNull() + expect(result.current.sortOrder).toBe('desc') }) }) describe('handleSort', () => { - it('should set sort field when called', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - - expect(result.current.sortField).toBe('name') - expect(result.current.sortOrder).toBe('desc') - }) - - it('should toggle sort order when same field is clicked twice', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - expect(result.current.sortOrder).toBe('desc') - - act(() => { - result.current.handleSort('name') - }) - expect(result.current.sortOrder).toBe('asc') - - act(() => { - result.current.handleSort('name') - }) - expect(result.current.sortOrder).toBe('desc') - }) - - it('should reset to desc when different field is selected', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - act(() => { - result.current.handleSort('name') - }) - expect(result.current.sortOrder).toBe('asc') - - act(() => { - result.current.handleSort('word_count') - }) - expect(result.current.sortField).toBe('word_count') - expect(result.current.sortOrder).toBe('desc') - }) - - it('should not change state when null is passed', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort(null) - }) - - expect(result.current.sortField).toBeNull() - }) - }) - - describe('sorting documents', () => { - const docs = [ - createMockDocument({ id: 'doc1', name: 'Banana', word_count: 200, hit_count: 5, created_at: 3000 }), - createMockDocument({ id: 'doc2', name: 'Apple', word_count: 100, hit_count: 10, created_at: 1000 }), - createMockDocument({ id: 'doc3', name: 'Cherry', word_count: 300, hit_count: 1, created_at: 2000 }), - ] - - it('should sort by name descending', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - - const names = result.current.sortedDocuments.map(d => d.name) - expect(names).toEqual(['Cherry', 'Banana', 'Apple']) - }) - - it('should sort by name ascending', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - act(() => { - result.current.handleSort('name') - }) - - const names = result.current.sortedDocuments.map(d => d.name) - expect(names).toEqual(['Apple', 'Banana', 'Cherry']) - }) - - it('should sort by word_count descending', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('word_count') - }) - - const counts = result.current.sortedDocuments.map(d => d.word_count) - expect(counts).toEqual([300, 200, 100]) - }) - - it('should sort by hit_count ascending', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) + it('should switch to desc when selecting a different field', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: '-created_at', + onRemoteSortChange, + })) act(() => { result.current.handleSort('hit_count') }) + + expect(onRemoteSortChange).toHaveBeenCalledWith('-hit_count') + }) + + it('should toggle desc -> asc when clicking active field', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: '-hit_count', + onRemoteSortChange, + })) + act(() => { result.current.handleSort('hit_count') }) - const counts = result.current.sortedDocuments.map(d => d.hit_count) - expect(counts).toEqual([1, 5, 10]) + expect(onRemoteSortChange).toHaveBeenCalledWith('hit_count') }) - it('should sort by created_at descending', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) + it('should toggle asc -> desc when clicking active field', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: 'created_at', + onRemoteSortChange, + })) act(() => { result.current.handleSort('created_at') }) - const times = result.current.sortedDocuments.map(d => d.created_at) - expect(times).toEqual([3000, 2000, 1000]) - }) - }) - - describe('status filtering', () => { - const docs = [ - createMockDocument({ id: 'doc1', display_status: 'available' }), - createMockDocument({ id: 'doc2', display_status: 'error' }), - createMockDocument({ id: 'doc3', display_status: 'available' }), - ] - - it('should not filter when statusFilterValue is empty', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - expect(result.current.sortedDocuments.length).toBe(3) + expect(onRemoteSortChange).toHaveBeenCalledWith('-created_at') }) - it('should not filter when statusFilterValue is all', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: 'all', - remoteSortValue: '', - }), - ) - - expect(result.current.sortedDocuments.length).toBe(3) - }) - }) - - describe('remoteSortValue reset', () => { - it('should reset sort state when remoteSortValue changes', () => { - const { result, rerender } = renderHook( - ({ remoteSortValue }) => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue, - }), - { initialProps: { remoteSortValue: 'initial' } }, - ) + it('should ignore null field', () => { + const onRemoteSortChange = vi.fn() + const { result } = renderHook(() => useDocumentSort({ + remoteSortValue: '-created_at', + onRemoteSortChange, + })) act(() => { - result.current.handleSort('name') - }) - act(() => { - result.current.handleSort('name') - }) - expect(result.current.sortField).toBe('name') - expect(result.current.sortOrder).toBe('asc') - - rerender({ remoteSortValue: 'changed' }) - - expect(result.current.sortField).toBeNull() - expect(result.current.sortOrder).toBe('desc') - }) - }) - - describe('edge cases', () => { - it('should handle documents with missing values', () => { - const docs = [ - createMockDocument({ id: 'doc1', name: undefined as unknown as string, word_count: undefined }), - createMockDocument({ id: 'doc2', name: 'Test', word_count: 100 }), - ] - - const { result } = renderHook(() => - useDocumentSort({ - documents: docs, - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') + result.current.handleSort(null) }) - expect(result.current.sortedDocuments.length).toBe(2) - }) - - it('should handle empty documents array', () => { - const { result } = renderHook(() => - useDocumentSort({ - documents: [], - statusFilterValue: '', - remoteSortValue: '', - }), - ) - - act(() => { - result.current.handleSort('name') - }) - - expect(result.current.sortedDocuments).toEqual([]) + expect(onRemoteSortChange).not.toHaveBeenCalled() }) }) }) diff --git a/web/app/components/datasets/documents/components/document-list/hooks/use-document-sort.ts b/web/app/components/datasets/documents/components/document-list/hooks/use-document-sort.ts index 98cf244f36..0e0b07db6f 100644 --- a/web/app/components/datasets/documents/components/document-list/hooks/use-document-sort.ts +++ b/web/app/components/datasets/documents/components/document-list/hooks/use-document-sort.ts @@ -1,102 +1,42 @@ -import type { SimpleDocumentDetail } from '@/models/datasets' -import { useCallback, useMemo, useRef, useState } from 'react' -import { normalizeStatusForQuery } from '@/app/components/datasets/documents/status-filter' +import { useCallback, useMemo } from 'react' -export type SortField = 'name' | 'word_count' | 'hit_count' | 'created_at' | null +type RemoteSortField = 'hit_count' | 'created_at' +const REMOTE_SORT_FIELDS = new Set(['hit_count', 'created_at']) + +export type SortField = RemoteSortField | null export type SortOrder = 'asc' | 'desc' -type LocalDoc = SimpleDocumentDetail & { percent?: number } - type UseDocumentSortOptions = { - documents: LocalDoc[] - statusFilterValue: string remoteSortValue: string + onRemoteSortChange: (nextSortValue: string) => void } export const useDocumentSort = ({ - documents, - statusFilterValue, remoteSortValue, + onRemoteSortChange, }: UseDocumentSortOptions) => { - const [sortField, setSortField] = useState(null) - const [sortOrder, setSortOrder] = useState('desc') - const prevRemoteSortValueRef = useRef(remoteSortValue) + const sortOrder: SortOrder = remoteSortValue.startsWith('-') ? 'desc' : 'asc' + const sortKey = remoteSortValue.startsWith('-') ? remoteSortValue.slice(1) : remoteSortValue - // Reset sort when remote sort changes - if (prevRemoteSortValueRef.current !== remoteSortValue) { - prevRemoteSortValueRef.current = remoteSortValue - setSortField(null) - setSortOrder('desc') - } + const sortField = useMemo(() => { + return REMOTE_SORT_FIELDS.has(sortKey as RemoteSortField) ? sortKey as RemoteSortField : null + }, [sortKey]) const handleSort = useCallback((field: SortField) => { - if (field === null) + if (!field) return if (sortField === field) { - setSortOrder(prev => prev === 'asc' ? 'desc' : 'asc') + const nextSortOrder = sortOrder === 'desc' ? 'asc' : 'desc' + onRemoteSortChange(nextSortOrder === 'desc' ? `-${field}` : field) + return } - else { - setSortField(field) - setSortOrder('desc') - } - }, [sortField]) - - const sortedDocuments = useMemo(() => { - let filteredDocs = documents - - if (statusFilterValue && statusFilterValue !== 'all') { - filteredDocs = filteredDocs.filter(doc => - typeof doc.display_status === 'string' - && normalizeStatusForQuery(doc.display_status) === statusFilterValue, - ) - } - - if (!sortField) - return filteredDocs - - const sortedDocs = [...filteredDocs].sort((a, b) => { - let aValue: string | number - let bValue: string | number - - switch (sortField) { - case 'name': - aValue = a.name?.toLowerCase() || '' - bValue = b.name?.toLowerCase() || '' - break - case 'word_count': - aValue = a.word_count || 0 - bValue = b.word_count || 0 - break - case 'hit_count': - aValue = a.hit_count || 0 - bValue = b.hit_count || 0 - break - case 'created_at': - aValue = a.created_at - bValue = b.created_at - break - default: - return 0 - } - - if (sortField === 'name') { - const result = (aValue as string).localeCompare(bValue as string) - return sortOrder === 'asc' ? result : -result - } - else { - const result = (aValue as number) - (bValue as number) - return sortOrder === 'asc' ? result : -result - } - }) - - return sortedDocs - }, [documents, sortField, sortOrder, statusFilterValue]) + onRemoteSortChange(`-${field}`) + }, [onRemoteSortChange, sortField, sortOrder]) return { sortField, sortOrder, handleSort, - sortedDocuments, } } diff --git a/web/app/components/datasets/documents/components/list.tsx b/web/app/components/datasets/documents/components/list.tsx index 3106f6c30b..e40e4c061b 100644 --- a/web/app/components/datasets/documents/components/list.tsx +++ b/web/app/components/datasets/documents/components/list.tsx @@ -14,7 +14,7 @@ import { useDatasetDetailContextWithSelector as useDatasetDetailContext } from ' import { ChunkingMode, DocumentActionType } from '@/models/datasets' import BatchAction from '../detail/completed/common/batch-action' import s from '../style.module.css' -import { DocumentTableRow, renderTdValue, SortHeader } from './document-list/components' +import { DocumentTableRow, SortHeader } from './document-list/components' import { useDocumentActions, useDocumentSelection, useDocumentSort } from './document-list/hooks' import RenameModal from './rename-modal' @@ -29,8 +29,8 @@ type DocumentListProps = { pagination: PaginationProps onUpdate: () => void onManageMetadata: () => void - statusFilterValue: string remoteSortValue: string + onSortChange: (value: string) => void } /** @@ -45,8 +45,8 @@ const DocumentList: FC = ({ pagination, onUpdate, onManageMetadata, - statusFilterValue, remoteSortValue, + onSortChange, }) => { const { t } = useTranslation() const datasetConfig = useDatasetDetailContext(s => s.dataset) @@ -55,10 +55,9 @@ const DocumentList: FC = ({ const isQAMode = chunkingMode === ChunkingMode.qa // Sorting - const { sortField, sortOrder, handleSort, sortedDocuments } = useDocumentSort({ - documents, - statusFilterValue, + const { sortField, sortOrder, handleSort } = useDocumentSort({ remoteSortValue, + onRemoteSortChange: onSortChange, }) // Selection @@ -71,7 +70,7 @@ const DocumentList: FC = ({ downloadableSelectedIds, clearSelection, } = useDocumentSelection({ - documents: sortedDocuments, + documents, selectedIds, onSelectedIdChange, }) @@ -135,24 +134,10 @@ const DocumentList: FC = ({ - + {t('list.table.header.fileName', { ns: 'datasetDocuments' })} {t('list.table.header.chunkingMode', { ns: 'datasetDocuments' })} - - - + {t('list.table.header.words', { ns: 'datasetDocuments' })} = ({ - {sortedDocuments.map((doc, index) => ( + {documents.map((doc, index) => ( = ({ } export default DocumentList - -export { renderTdValue } diff --git a/web/app/components/datasets/documents/detail/__tests__/index.spec.tsx b/web/app/components/datasets/documents/detail/__tests__/index.spec.tsx index ad8741a8e1..f01a64e34e 100644 --- a/web/app/components/datasets/documents/detail/__tests__/index.spec.tsx +++ b/web/app/components/datasets/documents/detail/__tests__/index.spec.tsx @@ -9,6 +9,7 @@ const mocks = vi.hoisted(() => { documentError: null as Error | null, documentMetadata: null as Record | null, media: 'desktop' as string, + searchParams: '' as string, } return { state, @@ -26,6 +27,7 @@ const mocks = vi.hoisted(() => { // --- External mocks --- vi.mock('next/navigation', () => ({ useRouter: () => ({ push: mocks.push }), + useSearchParams: () => new URLSearchParams(mocks.state.searchParams), })) vi.mock('@/hooks/use-breakpoints', () => ({ @@ -193,6 +195,7 @@ describe('DocumentDetail', () => { mocks.state.documentError = null mocks.state.documentMetadata = null mocks.state.media = 'desktop' + mocks.state.searchParams = '' }) afterEach(() => { @@ -286,15 +289,23 @@ describe('DocumentDetail', () => { }) it('should toggle metadata panel when button clicked', () => { - const { container } = render() + render() expect(screen.getByTestId('metadata')).toBeInTheDocument() - const svgs = container.querySelectorAll('svg') - const toggleBtn = svgs[svgs.length - 1].closest('button')! - fireEvent.click(toggleBtn) + fireEvent.click(screen.getByTestId('document-detail-metadata-toggle')) expect(screen.queryByTestId('metadata')).not.toBeInTheDocument() }) + it('should expose aria semantics for metadata toggle button', () => { + render() + const toggle = screen.getByTestId('document-detail-metadata-toggle') + expect(toggle).toHaveAttribute('aria-label') + expect(toggle).toHaveAttribute('aria-pressed', 'true') + + fireEvent.click(toggle) + expect(toggle).toHaveAttribute('aria-pressed', 'false') + }) + it('should pass correct props to Metadata', () => { render() const metadata = screen.getByTestId('metadata') @@ -305,20 +316,21 @@ describe('DocumentDetail', () => { describe('Navigation', () => { it('should navigate back when back button clicked', () => { - const { container } = render() - const backBtn = container.querySelector('svg')!.parentElement! - fireEvent.click(backBtn) + render() + fireEvent.click(screen.getByTestId('document-detail-back-button')) expect(mocks.push).toHaveBeenCalledWith('/datasets/ds-1/documents') }) + it('should expose aria label for back button', () => { + render() + expect(screen.getByTestId('document-detail-back-button')).toHaveAttribute('aria-label') + }) + it('should preserve query params when navigating back', () => { - const origLocation = window.location - window.history.pushState({}, '', '?page=2&status=active') - const { container } = render() - const backBtn = container.querySelector('svg')!.parentElement! - fireEvent.click(backBtn) + mocks.state.searchParams = 'page=2&status=active' + render() + fireEvent.click(screen.getByTestId('document-detail-back-button')) expect(mocks.push).toHaveBeenCalledWith('/datasets/ds-1/documents?page=2&status=active') - window.history.pushState({}, '', origLocation.href) }) }) diff --git a/web/app/components/datasets/documents/detail/index.tsx b/web/app/components/datasets/documents/detail/index.tsx index e147bf9aba..b6842605c6 100644 --- a/web/app/components/datasets/documents/detail/index.tsx +++ b/web/app/components/datasets/documents/detail/index.tsx @@ -1,8 +1,7 @@ 'use client' import type { FC } from 'react' import type { DataSourceInfo, FileItem, FullDocumentDetail, LegacyDataSourceInfo } from '@/models/datasets' -import { RiArrowLeftLine, RiLayoutLeft2Line, RiLayoutRight2Line } from '@remixicon/react' -import { useRouter } from 'next/navigation' +import { useRouter, useSearchParams } from 'next/navigation' import * as React from 'react' import { useMemo, useState } from 'react' import { useTranslation } from 'react-i18next' @@ -35,6 +34,7 @@ type DocumentDetailProps = { const DocumentDetail: FC = ({ datasetId, documentId }) => { const router = useRouter() + const searchParams = useSearchParams() const { t } = useTranslation() const media = useBreakpoints() @@ -98,11 +98,8 @@ const DocumentDetail: FC = ({ datasetId, documentId }) => { }) const backToPrev = () => { - // Preserve pagination and filter states when navigating back - const searchParams = new URLSearchParams(window.location.search) const queryString = searchParams.toString() - const separator = queryString ? '?' : '' - const backPath = `/datasets/${datasetId}/documents${separator}${queryString}` + const backPath = `/datasets/${datasetId}/documents${queryString ? `?${queryString}` : ''}` router.push(backPath) } @@ -152,6 +149,11 @@ const DocumentDetail: FC = ({ datasetId, documentId }) => { return chunkMode === ChunkingMode.parentChild && parentMode === 'full-doc' }, [documentDetail?.doc_form, parentMode]) + const backButtonLabel = t('operation.back', { ns: 'common' }) + const metadataToggleLabel = `${showMetadata + ? t('operation.close', { ns: 'common' }) + : t('operation.view', { ns: 'common' })} ${t('metadata.title', { ns: 'datasetDocuments' })}` + return ( = ({ datasetId, documentId }) => { >
-
- -
+ = ({ datasetId, documentId }) => { />
diff --git a/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.ts b/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.ts deleted file mode 100644 index e31d4ac547..0000000000 --- a/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.ts +++ /dev/null @@ -1,439 +0,0 @@ -import type { DocumentListQuery } from '../use-document-list-query-state' -import { act, renderHook } from '@testing-library/react' - -import { beforeEach, describe, expect, it, vi } from 'vitest' -import useDocumentListQueryState from '../use-document-list-query-state' - -const mockPush = vi.fn() -const mockSearchParams = new URLSearchParams() - -vi.mock('@/models/datasets', () => ({ - DisplayStatusList: [ - 'queuing', - 'indexing', - 'paused', - 'error', - 'available', - 'enabled', - 'disabled', - 'archived', - ], -})) - -vi.mock('next/navigation', () => ({ - useRouter: () => ({ push: mockPush }), - usePathname: () => '/datasets/test-id/documents', - useSearchParams: () => mockSearchParams, -})) - -describe('useDocumentListQueryState', () => { - beforeEach(() => { - vi.clearAllMocks() - // Reset mock search params to empty - for (const key of [...mockSearchParams.keys()]) - mockSearchParams.delete(key) - }) - - // Tests for parseParams (exposed via the query property) - describe('parseParams (via query)', () => { - it('should return default query when no search params present', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query).toEqual({ - page: 1, - limit: 10, - keyword: '', - status: 'all', - sort: '-created_at', - }) - }) - - it('should parse page from search params', () => { - mockSearchParams.set('page', '3') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.page).toBe(3) - }) - - it('should default page to 1 when page is zero', () => { - mockSearchParams.set('page', '0') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.page).toBe(1) - }) - - it('should default page to 1 when page is negative', () => { - mockSearchParams.set('page', '-5') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.page).toBe(1) - }) - - it('should default page to 1 when page is NaN', () => { - mockSearchParams.set('page', 'abc') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.page).toBe(1) - }) - - it('should parse limit from search params', () => { - mockSearchParams.set('limit', '50') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(50) - }) - - it('should default limit to 10 when limit is zero', () => { - mockSearchParams.set('limit', '0') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(10) - }) - - it('should default limit to 10 when limit exceeds 100', () => { - mockSearchParams.set('limit', '101') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(10) - }) - - it('should default limit to 10 when limit is negative', () => { - mockSearchParams.set('limit', '-1') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(10) - }) - - it('should accept limit at boundary 100', () => { - mockSearchParams.set('limit', '100') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(100) - }) - - it('should accept limit at boundary 1', () => { - mockSearchParams.set('limit', '1') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.limit).toBe(1) - }) - - it('should parse and decode keyword from search params', () => { - mockSearchParams.set('keyword', encodeURIComponent('hello world')) - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.keyword).toBe('hello world') - }) - - it('should return empty keyword when not present', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.keyword).toBe('') - }) - - it('should sanitize status from search params', () => { - mockSearchParams.set('status', 'available') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.status).toBe('available') - }) - - it('should fallback status to all for unknown status', () => { - mockSearchParams.set('status', 'badvalue') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.status).toBe('all') - }) - - it('should resolve active status alias to available', () => { - mockSearchParams.set('status', 'active') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.status).toBe('available') - }) - - it('should parse valid sort value from search params', () => { - mockSearchParams.set('sort', 'hit_count') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.sort).toBe('hit_count') - }) - - it('should default sort to -created_at for invalid sort value', () => { - mockSearchParams.set('sort', 'invalid_sort') - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.sort).toBe('-created_at') - }) - - it('should default sort to -created_at when not present', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.sort).toBe('-created_at') - }) - - it.each([ - '-created_at', - 'created_at', - '-hit_count', - 'hit_count', - ] as const)('should accept valid sort value %s', (sortValue) => { - mockSearchParams.set('sort', sortValue) - - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current.query.sort).toBe(sortValue) - }) - }) - - // Tests for updateQuery - describe('updateQuery', () => { - it('should call router.push with updated params when page is changed', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ page: 3 }) - }) - - expect(mockPush).toHaveBeenCalledTimes(1) - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('page=3') - }) - - it('should call router.push with scroll false', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ page: 2 }) - }) - - expect(mockPush).toHaveBeenCalledWith( - expect.any(String), - { scroll: false }, - ) - }) - - it('should set status in URL when status is not all', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ status: 'error' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('status=error') - }) - - it('should not set status in URL when status is all', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ status: 'all' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('status=') - }) - - it('should set sort in URL when sort is not the default -created_at', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ sort: 'hit_count' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('sort=hit_count') - }) - - it('should not set sort in URL when sort is default -created_at', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ sort: '-created_at' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('sort=') - }) - - it('should encode keyword in URL when keyword is provided', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ keyword: 'test query' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - // Source code applies encodeURIComponent before setting in URLSearchParams - expect(pushedUrl).toContain('keyword=') - const params = new URLSearchParams(pushedUrl.split('?')[1]) - // params.get decodes one layer, but the value was pre-encoded with encodeURIComponent - expect(decodeURIComponent(params.get('keyword')!)).toBe('test query') - }) - - it('should remove keyword from URL when keyword is empty', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ keyword: '' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('keyword=') - }) - - it('should sanitize invalid status to all and not include in URL', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ status: 'invalidstatus' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('status=') - }) - - it('should sanitize invalid sort to -created_at and not include in URL', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ sort: 'invalidsort' as DocumentListQuery['sort'] }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('sort=') - }) - - it('should omit page and limit when they are default and no keyword', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ page: 1, limit: 10 }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).not.toContain('page=') - expect(pushedUrl).not.toContain('limit=') - }) - - it('should include page and limit when page is greater than 1', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ page: 2 }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('page=2') - expect(pushedUrl).toContain('limit=10') - }) - - it('should include page and limit when limit is non-default', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ limit: 25 }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('page=1') - expect(pushedUrl).toContain('limit=25') - }) - - it('should include page and limit when keyword is provided', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ keyword: 'search' }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toContain('page=1') - expect(pushedUrl).toContain('limit=10') - }) - - it('should use pathname prefix in pushed URL', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({ page: 2 }) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toMatch(/^\/datasets\/test-id\/documents/) - }) - - it('should push path without query string when all values are defaults', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.updateQuery({}) - }) - - const pushedUrl = mockPush.mock.calls[0][0] as string - expect(pushedUrl).toBe('/datasets/test-id/documents') - }) - }) - - // Tests for resetQuery - describe('resetQuery', () => { - it('should push URL with default query params when called', () => { - mockSearchParams.set('page', '5') - mockSearchParams.set('status', 'error') - - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.resetQuery() - }) - - expect(mockPush).toHaveBeenCalledTimes(1) - const pushedUrl = mockPush.mock.calls[0][0] as string - // Default query has all defaults, so no params should be in the URL - expect(pushedUrl).toBe('/datasets/test-id/documents') - }) - - it('should call router.push with scroll false when resetting', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - act(() => { - result.current.resetQuery() - }) - - expect(mockPush).toHaveBeenCalledWith( - expect.any(String), - { scroll: false }, - ) - }) - }) - - // Tests for return value stability - describe('return value', () => { - it('should return query, updateQuery, and resetQuery', () => { - const { result } = renderHook(() => useDocumentListQueryState()) - - expect(result.current).toHaveProperty('query') - expect(result.current).toHaveProperty('updateQuery') - expect(result.current).toHaveProperty('resetQuery') - expect(typeof result.current.updateQuery).toBe('function') - expect(typeof result.current.resetQuery).toBe('function') - }) - }) -}) diff --git a/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.tsx b/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.tsx new file mode 100644 index 0000000000..5879e72782 --- /dev/null +++ b/web/app/components/datasets/documents/hooks/__tests__/use-document-list-query-state.spec.tsx @@ -0,0 +1,426 @@ +import type { DocumentListQuery } from '../use-document-list-query-state' +import { act, waitFor } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { renderHookWithNuqs } from '@/test/nuqs-testing' +import { useDocumentListQueryState } from '../use-document-list-query-state' + +vi.mock('@/models/datasets', () => ({ + DisplayStatusList: [ + 'queuing', + 'indexing', + 'paused', + 'error', + 'available', + 'enabled', + 'disabled', + 'archived', + ], +})) + +const renderWithAdapter = (searchParams = '') => { + return renderHookWithNuqs(() => useDocumentListQueryState(), { searchParams }) +} + +describe('useDocumentListQueryState', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + describe('query parsing', () => { + it('should return default query when no search params present', () => { + const { result } = renderWithAdapter() + + expect(result.current.query).toEqual({ + page: 1, + limit: 10, + keyword: '', + status: 'all', + sort: '-created_at', + }) + }) + + it('should parse page from search params', () => { + const { result } = renderWithAdapter('?page=3') + expect(result.current.query.page).toBe(3) + }) + + it('should default page to 1 when page is zero', () => { + const { result } = renderWithAdapter('?page=0') + expect(result.current.query.page).toBe(1) + }) + + it('should default page to 1 when page is negative', () => { + const { result } = renderWithAdapter('?page=-5') + expect(result.current.query.page).toBe(1) + }) + + it('should default page to 1 when page is NaN', () => { + const { result } = renderWithAdapter('?page=abc') + expect(result.current.query.page).toBe(1) + }) + + it('should parse limit from search params', () => { + const { result } = renderWithAdapter('?limit=50') + expect(result.current.query.limit).toBe(50) + }) + + it('should default limit to 10 when limit is zero', () => { + const { result } = renderWithAdapter('?limit=0') + expect(result.current.query.limit).toBe(10) + }) + + it('should default limit to 10 when limit exceeds 100', () => { + const { result } = renderWithAdapter('?limit=101') + expect(result.current.query.limit).toBe(10) + }) + + it('should default limit to 10 when limit is negative', () => { + const { result } = renderWithAdapter('?limit=-1') + expect(result.current.query.limit).toBe(10) + }) + + it('should accept limit at boundary 100', () => { + const { result } = renderWithAdapter('?limit=100') + expect(result.current.query.limit).toBe(100) + }) + + it('should accept limit at boundary 1', () => { + const { result } = renderWithAdapter('?limit=1') + expect(result.current.query.limit).toBe(1) + }) + + it('should parse keyword from search params', () => { + const { result } = renderWithAdapter('?keyword=hello+world') + expect(result.current.query.keyword).toBe('hello world') + }) + + it('should preserve legacy double-encoded keyword text after URL decoding', () => { + const { result } = renderWithAdapter('?keyword=test%2520query') + expect(result.current.query.keyword).toBe('test%20query') + }) + + it('should return empty keyword when not present', () => { + const { result } = renderWithAdapter() + expect(result.current.query.keyword).toBe('') + }) + + it('should sanitize status from search params', () => { + const { result } = renderWithAdapter('?status=available') + expect(result.current.query.status).toBe('available') + }) + + it('should fallback status to all for unknown status', () => { + const { result } = renderWithAdapter('?status=badvalue') + expect(result.current.query.status).toBe('all') + }) + + it('should resolve active status alias to available', () => { + const { result } = renderWithAdapter('?status=active') + expect(result.current.query.status).toBe('available') + }) + + it('should parse valid sort value from search params', () => { + const { result } = renderWithAdapter('?sort=hit_count') + expect(result.current.query.sort).toBe('hit_count') + }) + + it('should default sort to -created_at for invalid sort value', () => { + const { result } = renderWithAdapter('?sort=invalid_sort') + expect(result.current.query.sort).toBe('-created_at') + }) + + it('should default sort to -created_at when not present', () => { + const { result } = renderWithAdapter() + expect(result.current.query.sort).toBe('-created_at') + }) + + it.each([ + '-created_at', + 'created_at', + '-hit_count', + 'hit_count', + ] as const)('should accept valid sort value %s', (sortValue) => { + const { result } = renderWithAdapter(`?sort=${sortValue}`) + expect(result.current.query.sort).toBe(sortValue) + }) + }) + + describe('updateQuery', () => { + it('should update page in state when page is changed', () => { + const { result } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ page: 3 }) + }) + + expect(result.current.query.page).toBe(3) + }) + + it('should sync page to URL with push history', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ page: 2 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('page')).toBe('2') + expect(update.options.history).toBe('push') + }) + + it('should set status in URL when status is not all', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ status: 'error' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('status')).toBe('error') + }) + + it('should not set status in URL when status is all', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ status: 'all' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('status')).toBe(false) + }) + + it('should set sort in URL when sort is not the default -created_at', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ sort: 'hit_count' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('sort')).toBe('hit_count') + }) + + it('should not set sort in URL when sort is default -created_at', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ sort: '-created_at' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('sort')).toBe(false) + }) + + it('should set keyword in URL when keyword is provided', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ keyword: 'test query' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('keyword')).toBe('test query') + expect(update.options.history).toBe('replace') + }) + + it('should use replace history when keyword update also resets page', async () => { + const { result, onUrlUpdate } = renderWithAdapter('?page=3') + + act(() => { + result.current.updateQuery({ keyword: 'hello', page: 1 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('keyword')).toBe('hello') + expect(update.searchParams.has('page')).toBe(false) + expect(update.options.history).toBe('replace') + }) + + it('should remove keyword from URL when keyword is empty', async () => { + const { result, onUrlUpdate } = renderWithAdapter('?keyword=existing') + + act(() => { + result.current.updateQuery({ keyword: '' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('keyword')).toBe(false) + expect(update.options.history).toBe('replace') + }) + + it('should remove keyword from URL when keyword contains only whitespace', async () => { + const { result, onUrlUpdate } = renderWithAdapter('?keyword=existing') + + act(() => { + result.current.updateQuery({ keyword: ' ' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('keyword')).toBe(false) + expect(result.current.query.keyword).toBe('') + }) + + it('should preserve literal percent-encoded-like keyword values', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ keyword: '%2F' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('keyword')).toBe('%2F') + expect(result.current.query.keyword).toBe('%2F') + }) + + it('should keep keyword text unchanged when updating query from legacy URL', async () => { + const { result, onUrlUpdate } = renderWithAdapter('?keyword=test%2520query') + + act(() => { + result.current.updateQuery({ page: 2 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + expect(result.current.query.keyword).toBe('test%20query') + }) + + it('should sanitize invalid status to all and not include in URL', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ status: 'invalidstatus' }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('status')).toBe(false) + }) + + it('should sanitize invalid sort to -created_at and not include in URL', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ sort: 'invalidsort' as DocumentListQuery['sort'] }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('sort')).toBe(false) + }) + + it('should not include page in URL when page is default', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ page: 1 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('page')).toBe(false) + }) + + it('should include page in URL when page is greater than 1', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ page: 2 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('page')).toBe('2') + }) + + it('should include limit in URL when limit is non-default', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ limit: 25 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.get('limit')).toBe('25') + }) + + it('should sanitize invalid page to default and omit page from URL', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ page: -1 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('page')).toBe(false) + expect(result.current.query.page).toBe(1) + }) + + it('should sanitize invalid limit to default and omit limit from URL', async () => { + const { result, onUrlUpdate } = renderWithAdapter() + + act(() => { + result.current.updateQuery({ limit: 999 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('limit')).toBe(false) + expect(result.current.query.limit).toBe(10) + }) + }) + + describe('resetQuery', () => { + it('should reset all values to defaults', () => { + const { result } = renderWithAdapter('?page=5&status=error&sort=hit_count') + + act(() => { + result.current.resetQuery() + }) + + expect(result.current.query).toEqual({ + page: 1, + limit: 10, + keyword: '', + status: 'all', + sort: '-created_at', + }) + }) + + it('should clear all params from URL when called', async () => { + const { result, onUrlUpdate } = renderWithAdapter('?page=5&status=error') + + act(() => { + result.current.resetQuery() + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls[onUrlUpdate.mock.calls.length - 1][0] + expect(update.searchParams.has('page')).toBe(false) + expect(update.searchParams.has('status')).toBe(false) + }) + }) + + describe('return value', () => { + it('should return query, updateQuery, and resetQuery', () => { + const { result } = renderWithAdapter() + + expect(result.current).toHaveProperty('query') + expect(result.current).toHaveProperty('updateQuery') + expect(result.current).toHaveProperty('resetQuery') + expect(typeof result.current.updateQuery).toBe('function') + expect(typeof result.current.resetQuery).toBe('function') + }) + }) +}) diff --git a/web/app/components/datasets/documents/hooks/__tests__/use-documents-page-state.spec.ts b/web/app/components/datasets/documents/hooks/__tests__/use-documents-page-state.spec.ts index 34911e9e9c..e0dbee6660 100644 --- a/web/app/components/datasets/documents/hooks/__tests__/use-documents-page-state.spec.ts +++ b/web/app/components/datasets/documents/hooks/__tests__/use-documents-page-state.spec.ts @@ -1,12 +1,10 @@ import type { DocumentListQuery } from '../use-document-list-query-state' -import type { DocumentListResponse } from '@/models/datasets' import { act, renderHook } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { useDocumentsPageState } from '../use-documents-page-state' const mockUpdateQuery = vi.fn() -const mockResetQuery = vi.fn() let mockQuery: DocumentListQuery = { page: 1, limit: 10, keyword: '', status: 'all', sort: '-created_at' } vi.mock('@/models/datasets', () => ({ @@ -22,151 +20,70 @@ vi.mock('@/models/datasets', () => ({ ], })) -vi.mock('next/navigation', () => ({ - useRouter: () => ({ push: vi.fn() }), - usePathname: () => '/datasets/test-id/documents', - useSearchParams: () => new URLSearchParams(), -})) - -// Mock ahooks debounce utilities: required because tests capture the debounce -// callback reference to invoke it synchronously, bypassing real timer delays. -let capturedDebounceFnCallback: (() => void) | null = null - vi.mock('ahooks', () => ({ useDebounce: (value: unknown, _options?: { wait?: number }) => value, - useDebounceFn: (fn: () => void, _options?: { wait?: number }) => { - capturedDebounceFnCallback = fn - return { run: fn, cancel: vi.fn(), flush: vi.fn() } - }, })) -// Mock the dependent hook -vi.mock('../use-document-list-query-state', () => ({ - default: () => ({ - query: mockQuery, - updateQuery: mockUpdateQuery, - resetQuery: mockResetQuery, - }), -})) - -// Factory for creating DocumentListResponse test data -function createDocumentListResponse(overrides: Partial = {}): DocumentListResponse { +vi.mock('../use-document-list-query-state', async () => { + const React = await import('react') return { - data: [], - has_more: false, - total: 0, - page: 1, - limit: 10, - ...overrides, + useDocumentListQueryState: () => { + const [query, setQuery] = React.useState(mockQuery) + return { + query, + updateQuery: (updates: Partial) => { + mockUpdateQuery(updates) + setQuery(prev => ({ ...prev, ...updates })) + }, + } + }, } -} - -// Factory for creating a minimal document item -function createDocumentItem(overrides: Record = {}) { - return { - id: `doc-${Math.random().toString(36).slice(2, 8)}`, - name: 'test-doc.txt', - indexing_status: 'completed' as string, - display_status: 'available' as string, - enabled: true, - archived: false, - word_count: 100, - created_at: Date.now(), - updated_at: Date.now(), - created_from: 'web' as const, - created_by: 'user-1', - dataset_process_rule_id: 'rule-1', - doc_form: 'text_model' as const, - doc_language: 'en', - position: 1, - data_source_type: 'upload_file', - ...overrides, - } -} +}) describe('useDocumentsPageState', () => { beforeEach(() => { vi.clearAllMocks() - capturedDebounceFnCallback = null mockQuery = { page: 1, limit: 10, keyword: '', status: 'all', sort: '-created_at' } }) // Initial state verification describe('initial state', () => { - it('should return correct initial search state', () => { + it('should return correct initial query-derived state', () => { const { result } = renderHook(() => useDocumentsPageState()) expect(result.current.inputValue).toBe('') - expect(result.current.searchValue).toBe('') expect(result.current.debouncedSearchValue).toBe('') - }) - - it('should return correct initial filter and sort state', () => { - const { result } = renderHook(() => useDocumentsPageState()) - expect(result.current.statusFilterValue).toBe('all') expect(result.current.sortValue).toBe('-created_at') expect(result.current.normalizedStatusFilterValue).toBe('all') - }) - - it('should return correct initial pagination state', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - // page is query.page - 1 = 0 expect(result.current.currPage).toBe(0) expect(result.current.limit).toBe(10) - }) - - it('should return correct initial selection state', () => { - const { result } = renderHook(() => useDocumentsPageState()) - expect(result.current.selectedIds).toEqual([]) }) - it('should return correct initial polling state', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - expect(result.current.timerCanRun).toBe(true) - }) - - it('should initialize from query when query has keyword', () => { - mockQuery = { ...mockQuery, keyword: 'initial search' } + it('should initialize from non-default query values', () => { + mockQuery = { + page: 3, + limit: 25, + keyword: 'initial', + status: 'enabled', + sort: 'hit_count', + } const { result } = renderHook(() => useDocumentsPageState()) - expect(result.current.inputValue).toBe('initial search') - expect(result.current.searchValue).toBe('initial search') - }) - - it('should initialize pagination from query with non-default page', () => { - mockQuery = { ...mockQuery, page: 3, limit: 25 } - - const { result } = renderHook(() => useDocumentsPageState()) - - expect(result.current.currPage).toBe(2) // page - 1 + expect(result.current.inputValue).toBe('initial') + expect(result.current.currPage).toBe(2) expect(result.current.limit).toBe(25) - }) - - it('should initialize status filter from query', () => { - mockQuery = { ...mockQuery, status: 'error' } - - const { result } = renderHook(() => useDocumentsPageState()) - - expect(result.current.statusFilterValue).toBe('error') - }) - - it('should initialize sort from query', () => { - mockQuery = { ...mockQuery, sort: 'hit_count' } - - const { result } = renderHook(() => useDocumentsPageState()) - + expect(result.current.statusFilterValue).toBe('enabled') + expect(result.current.normalizedStatusFilterValue).toBe('available') expect(result.current.sortValue).toBe('hit_count') }) }) // Handler behaviors describe('handleInputChange', () => { - it('should update input value when called', () => { + it('should update keyword and reset page', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -174,30 +91,59 @@ describe('useDocumentsPageState', () => { }) expect(result.current.inputValue).toBe('new value') + expect(mockUpdateQuery).toHaveBeenCalledWith({ keyword: 'new value', page: 1 }) }) - it('should trigger debounced search callback when called', () => { + it('should clear selected ids when keyword changes', () => { const { result } = renderHook(() => useDocumentsPageState()) - // First call sets inputValue and triggers the debounced fn act(() => { - result.current.handleInputChange('search term') + result.current.setSelectedIds(['doc-1']) + }) + expect(result.current.selectedIds).toEqual(['doc-1']) + + act(() => { + result.current.handleInputChange('keyword') }) - // The debounced fn captures inputValue from its render closure. - // After re-render with new inputValue, calling the captured callback again - // should reflect the updated state. + expect(result.current.selectedIds).toEqual([]) + }) + + it('should keep selected ids when keyword is unchanged', () => { + mockQuery = { ...mockQuery, keyword: 'same' } + const { result } = renderHook(() => useDocumentsPageState()) + act(() => { - if (capturedDebounceFnCallback) - capturedDebounceFnCallback() + result.current.setSelectedIds(['doc-1']) }) - expect(result.current.searchValue).toBe('search term') + act(() => { + result.current.handleInputChange('same') + }) + + expect(result.current.selectedIds).toEqual(['doc-1']) + expect(mockUpdateQuery).toHaveBeenCalledWith({ keyword: 'same', page: 1 }) }) }) describe('handleStatusFilterChange', () => { - it('should update status filter value when called with valid status', () => { + it('should sanitize status, reset page, and clear selection', () => { + const { result } = renderHook(() => useDocumentsPageState()) + + act(() => { + result.current.setSelectedIds(['doc-1']) + }) + + act(() => { + result.current.handleStatusFilterChange('invalid') + }) + + expect(result.current.statusFilterValue).toBe('all') + expect(result.current.selectedIds).toEqual([]) + expect(mockUpdateQuery).toHaveBeenCalledWith({ status: 'all', page: 1 }) + }) + + it('should update to valid status value', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -205,61 +151,23 @@ describe('useDocumentsPageState', () => { }) expect(result.current.statusFilterValue).toBe('error') - }) - - it('should reset page to 0 when status filter changes', () => { - mockQuery = { ...mockQuery, page: 3 } - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('error') - }) - - expect(result.current.currPage).toBe(0) - }) - - it('should call updateQuery with sanitized status and page 1', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('error') - }) - expect(mockUpdateQuery).toHaveBeenCalledWith({ status: 'error', page: 1 }) }) - - it('should sanitize invalid status to all', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('invalid') - }) - - expect(result.current.statusFilterValue).toBe('all') - expect(mockUpdateQuery).toHaveBeenCalledWith({ status: 'all', page: 1 }) - }) }) describe('handleStatusFilterClear', () => { - it('should set status to all and reset page when status is not all', () => { + it('should reset status to all when status is not all', () => { + mockQuery = { ...mockQuery, status: 'error' } const { result } = renderHook(() => useDocumentsPageState()) - // First set a non-all status - act(() => { - result.current.handleStatusFilterChange('error') - }) - vi.clearAllMocks() - - // Then clear act(() => { result.current.handleStatusFilterClear() }) - expect(result.current.statusFilterValue).toBe('all') expect(mockUpdateQuery).toHaveBeenCalledWith({ status: 'all', page: 1 }) }) - it('should not call updateQuery when status is already all', () => { + it('should do nothing when status is already all', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -271,7 +179,7 @@ describe('useDocumentsPageState', () => { }) describe('handleSortChange', () => { - it('should update sort value and call updateQuery when value changes', () => { + it('should update sort and reset page when sort changes', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -282,18 +190,7 @@ describe('useDocumentsPageState', () => { expect(mockUpdateQuery).toHaveBeenCalledWith({ sort: 'hit_count', page: 1 }) }) - it('should reset page to 0 when sort changes', () => { - mockQuery = { ...mockQuery, page: 5 } - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleSortChange('hit_count') - }) - - expect(result.current.currPage).toBe(0) - }) - - it('should not call updateQuery when sort value is same as current', () => { + it('should ignore sort update when value is unchanged', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -304,8 +201,8 @@ describe('useDocumentsPageState', () => { }) }) - describe('handlePageChange', () => { - it('should update current page and call updateQuery', () => { + describe('pagination handlers', () => { + it('should update page with one-based value', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -313,23 +210,10 @@ describe('useDocumentsPageState', () => { }) expect(result.current.currPage).toBe(2) - expect(mockUpdateQuery).toHaveBeenCalledWith({ page: 3 }) // newPage + 1 + expect(mockUpdateQuery).toHaveBeenCalledWith({ page: 3 }) }) - it('should handle page 0 (first page)', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handlePageChange(0) - }) - - expect(result.current.currPage).toBe(0) - expect(mockUpdateQuery).toHaveBeenCalledWith({ page: 1 }) - }) - }) - - describe('handleLimitChange', () => { - it('should update limit, reset page to 0, and call updateQuery', () => { + it('should update limit and reset page', () => { const { result } = renderHook(() => useDocumentsPageState()) act(() => { @@ -342,359 +226,29 @@ describe('useDocumentsPageState', () => { }) }) - // Selection state - describe('selection state', () => { - it('should update selectedIds via setSelectedIds', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.setSelectedIds(['doc-1', 'doc-2']) - }) - - expect(result.current.selectedIds).toEqual(['doc-1', 'doc-2']) - }) - }) - - // Polling state management - describe('updatePollingState', () => { - it('should not update timer when documentsRes is undefined', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState(undefined) - }) - - // timerCanRun remains true (initial value) - expect(result.current.timerCanRun).toBe(true) - }) - - it('should not update timer when documentsRes.data is undefined', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState({ data: undefined } as unknown as DocumentListResponse) - }) - - expect(result.current.timerCanRun).toBe(true) - }) - - it('should set timerCanRun to false when all documents are completed and status filter is all', () => { - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'completed' }), - createDocumentItem({ indexing_status: 'completed' }), - ] as DocumentListResponse['data'], - total: 2, - }) - - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState(response) - }) - - expect(result.current.timerCanRun).toBe(false) - }) - - it('should set timerCanRun to true when some documents are not completed', () => { - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'completed' }), - createDocumentItem({ indexing_status: 'indexing' }), - ] as DocumentListResponse['data'], - total: 2, - }) - - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState(response) - }) - - expect(result.current.timerCanRun).toBe(true) - }) - - it('should count paused documents as completed for polling purposes', () => { - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'paused' }), - createDocumentItem({ indexing_status: 'completed' }), - ] as DocumentListResponse['data'], - total: 2, - }) - - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState(response) - }) - - // All docs are "embedded" (completed, paused, error), so hasIncomplete = false - // statusFilter is 'all', so shouldForcePolling = false - expect(result.current.timerCanRun).toBe(false) - }) - - it('should count error documents as completed for polling purposes', () => { - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'error' }), - createDocumentItem({ indexing_status: 'completed' }), - ] as DocumentListResponse['data'], - total: 2, - }) - - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.updatePollingState(response) - }) - - expect(result.current.timerCanRun).toBe(false) - }) - - it('should force polling when status filter is a transient status (queuing)', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - // Set status filter to queuing - act(() => { - result.current.handleStatusFilterChange('queuing') - }) - - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'completed' }), - ] as DocumentListResponse['data'], - total: 1, - }) - - act(() => { - result.current.updatePollingState(response) - }) - - // shouldForcePolling = true (queuing is transient), hasIncomplete = false - // timerCanRun = true || false = true - expect(result.current.timerCanRun).toBe(true) - }) - - it('should force polling when status filter is indexing', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('indexing') - }) - - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'completed' }), - ] as DocumentListResponse['data'], - total: 1, - }) - - act(() => { - result.current.updatePollingState(response) - }) - - expect(result.current.timerCanRun).toBe(true) - }) - - it('should force polling when status filter is paused', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('paused') - }) - - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'paused' }), - ] as DocumentListResponse['data'], - total: 1, - }) - - act(() => { - result.current.updatePollingState(response) - }) - - expect(result.current.timerCanRun).toBe(true) - }) - - it('should not force polling when status filter is a non-transient status (error)', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('error') - }) - - const response = createDocumentListResponse({ - data: [ - createDocumentItem({ indexing_status: 'error' }), - ] as DocumentListResponse['data'], - total: 1, - }) - - act(() => { - result.current.updatePollingState(response) - }) - - // shouldForcePolling = false (error is not transient), hasIncomplete = false (error is embedded) - expect(result.current.timerCanRun).toBe(false) - }) - - it('should set timerCanRun to true when data is empty and filter is transient', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('indexing') - }) - - const response = createDocumentListResponse({ data: [] as DocumentListResponse['data'], total: 0 }) - - act(() => { - result.current.updatePollingState(response) - }) - - // shouldForcePolling = true (indexing is transient), hasIncomplete = false (0 !== 0 is false) - expect(result.current.timerCanRun).toBe(true) - }) - }) - - // Page adjustment - describe('adjustPageForTotal', () => { - it('should not adjust page when documentsRes is undefined', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.adjustPageForTotal(undefined) - }) - - expect(result.current.currPage).toBe(0) - }) - - it('should not adjust page when currPage is within total pages', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - const response = createDocumentListResponse({ total: 20 }) - - act(() => { - result.current.adjustPageForTotal(response) - }) - - // currPage is 0, totalPages is 2, so no adjustment needed - expect(result.current.currPage).toBe(0) - }) - - it('should adjust page to last page when currPage exceeds total pages', () => { - mockQuery = { ...mockQuery, page: 6 } - const { result } = renderHook(() => useDocumentsPageState()) - - // currPage should be 5 (page - 1) - expect(result.current.currPage).toBe(5) - - const response = createDocumentListResponse({ total: 30 }) // 30/10 = 3 pages - - act(() => { - result.current.adjustPageForTotal(response) - }) - - // currPage (5) + 1 > totalPages (3), so adjust to totalPages - 1 = 2 - expect(result.current.currPage).toBe(2) - expect(mockUpdateQuery).toHaveBeenCalledWith({ page: 3 }) // handlePageChange passes newPage + 1 - }) - - it('should adjust page to 0 when total is 0 and currPage > 0', () => { - mockQuery = { ...mockQuery, page: 3 } - const { result } = renderHook(() => useDocumentsPageState()) - - const response = createDocumentListResponse({ total: 0 }) - - act(() => { - result.current.adjustPageForTotal(response) - }) - - // totalPages = 0, so adjust to max(0 - 1, 0) = 0 - expect(result.current.currPage).toBe(0) - expect(mockUpdateQuery).toHaveBeenCalledWith({ page: 1 }) - }) - - it('should not adjust page when currPage is 0 even if total is 0', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - const response = createDocumentListResponse({ total: 0 }) - - act(() => { - result.current.adjustPageForTotal(response) - }) - - // currPage is 0, condition is currPage > 0 so no adjustment - expect(mockUpdateQuery).not.toHaveBeenCalled() - }) - }) - - // Normalized status filter value - describe('normalizedStatusFilterValue', () => { - it('should return all for default status', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - expect(result.current.normalizedStatusFilterValue).toBe('all') - }) - - it('should normalize enabled to available', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('enabled') - }) - - expect(result.current.normalizedStatusFilterValue).toBe('available') - }) - - it('should return non-aliased status as-is', () => { - const { result } = renderHook(() => useDocumentsPageState()) - - act(() => { - result.current.handleStatusFilterChange('error') - }) - - expect(result.current.normalizedStatusFilterValue).toBe('error') - }) - }) - // Return value shape describe('return value', () => { it('should return all expected properties', () => { const { result } = renderHook(() => useDocumentsPageState()) - // Search state expect(result.current).toHaveProperty('inputValue') - expect(result.current).toHaveProperty('searchValue') expect(result.current).toHaveProperty('debouncedSearchValue') expect(result.current).toHaveProperty('handleInputChange') - - // Filter & sort state expect(result.current).toHaveProperty('statusFilterValue') expect(result.current).toHaveProperty('sortValue') expect(result.current).toHaveProperty('normalizedStatusFilterValue') expect(result.current).toHaveProperty('handleStatusFilterChange') expect(result.current).toHaveProperty('handleStatusFilterClear') expect(result.current).toHaveProperty('handleSortChange') - - // Pagination state expect(result.current).toHaveProperty('currPage') expect(result.current).toHaveProperty('limit') expect(result.current).toHaveProperty('handlePageChange') expect(result.current).toHaveProperty('handleLimitChange') - - // Selection state expect(result.current).toHaveProperty('selectedIds') expect(result.current).toHaveProperty('setSelectedIds') - - // Polling state - expect(result.current).toHaveProperty('timerCanRun') - expect(result.current).toHaveProperty('updatePollingState') - expect(result.current).toHaveProperty('adjustPageForTotal') }) - it('should have function types for all handlers', () => { + it('should expose function handlers', () => { const { result } = renderHook(() => useDocumentsPageState()) expect(typeof result.current.handleInputChange).toBe('function') @@ -704,8 +258,6 @@ describe('useDocumentsPageState', () => { expect(typeof result.current.handlePageChange).toBe('function') expect(typeof result.current.handleLimitChange).toBe('function') expect(typeof result.current.setSelectedIds).toBe('function') - expect(typeof result.current.updatePollingState).toBe('function') - expect(typeof result.current.adjustPageForTotal).toBe('function') }) }) }) diff --git a/web/app/components/datasets/documents/hooks/use-document-list-query-state.ts b/web/app/components/datasets/documents/hooks/use-document-list-query-state.ts index 505f15efc0..60717d532c 100644 --- a/web/app/components/datasets/documents/hooks/use-document-list-query-state.ts +++ b/web/app/components/datasets/documents/hooks/use-document-list-query-state.ts @@ -1,6 +1,6 @@ -import type { ReadonlyURLSearchParams } from 'next/navigation' +import type { inferParserType } from 'nuqs' import type { SortType } from '@/service/datasets' -import { usePathname, useRouter, useSearchParams } from 'next/navigation' +import { createParser, parseAsString, throttle, useQueryStates } from 'nuqs' import { useCallback, useMemo } from 'react' import { sanitizeStatusValue } from '../status-filter' @@ -13,99 +13,87 @@ const sanitizeSortValue = (value?: string | null): SortType => { return (ALLOWED_SORT_VALUES.includes(value as SortType) ? value : '-created_at') as SortType } -export type DocumentListQuery = { - page: number - limit: number - keyword: string - status: string - sort: SortType +const sanitizePageValue = (value: number): number => { + return Number.isInteger(value) && value > 0 ? value : 1 } -const DEFAULT_QUERY: DocumentListQuery = { - page: 1, - limit: 10, - keyword: '', - status: 'all', - sort: '-created_at', +const sanitizeLimitValue = (value: number): number => { + return Number.isInteger(value) && value > 0 && value <= 100 ? value : 10 } -// Parse the query parameters from the URL search string. -function parseParams(params: ReadonlyURLSearchParams): DocumentListQuery { - const page = Number.parseInt(params.get('page') || '1', 10) - const limit = Number.parseInt(params.get('limit') || '10', 10) - const keyword = params.get('keyword') || '' - const status = sanitizeStatusValue(params.get('status')) - const sort = sanitizeSortValue(params.get('sort')) +const parseAsPage = createParser({ + parse: (value) => { + const n = Number.parseInt(value, 10) + return Number.isNaN(n) || n <= 0 ? null : n + }, + serialize: value => value.toString(), +}).withDefault(1) - return { - page: page > 0 ? page : 1, - limit: (limit > 0 && limit <= 100) ? limit : 10, - keyword: keyword ? decodeURIComponent(keyword) : '', - status, - sort, - } +const parseAsLimit = createParser({ + parse: (value) => { + const n = Number.parseInt(value, 10) + return Number.isNaN(n) || n <= 0 || n > 100 ? null : n + }, + serialize: value => value.toString(), +}).withDefault(10) + +const parseAsDocStatus = createParser({ + parse: value => sanitizeStatusValue(value), + serialize: value => value, +}).withDefault('all') + +const parseAsDocSort = createParser({ + parse: value => sanitizeSortValue(value), + serialize: value => value, +}).withDefault('-created_at' as SortType) + +const parseAsKeyword = parseAsString.withDefault('') + +export const documentListParsers = { + page: parseAsPage, + limit: parseAsLimit, + keyword: parseAsKeyword, + status: parseAsDocStatus, + sort: parseAsDocSort, } -// Update the URL search string with the given query parameters. -function updateSearchParams(query: DocumentListQuery, searchParams: URLSearchParams) { - const { page, limit, keyword, status, sort } = query || {} +export type DocumentListQuery = inferParserType - const hasNonDefaultParams = (page && page > 1) || (limit && limit !== 10) || (keyword && keyword.trim()) +// Search input updates can be frequent; throttle URL writes to reduce history/api churn. +const KEYWORD_URL_UPDATE_THROTTLE = throttle(300) - if (hasNonDefaultParams) { - searchParams.set('page', (page || 1).toString()) - searchParams.set('limit', (limit || 10).toString()) - } - else { - searchParams.delete('page') - searchParams.delete('limit') - } +export function useDocumentListQueryState() { + const [query, setQuery] = useQueryStates(documentListParsers) - if (keyword && keyword.trim()) - searchParams.set('keyword', encodeURIComponent(keyword)) - else - searchParams.delete('keyword') - - const sanitizedStatus = sanitizeStatusValue(status) - if (sanitizedStatus && sanitizedStatus !== 'all') - searchParams.set('status', sanitizedStatus) - else - searchParams.delete('status') - - const sanitizedSort = sanitizeSortValue(sort) - if (sanitizedSort !== '-created_at') - searchParams.set('sort', sanitizedSort) - else - searchParams.delete('sort') -} - -function useDocumentListQueryState() { - const searchParams = useSearchParams() - const query = useMemo(() => parseParams(searchParams), [searchParams]) - - const router = useRouter() - const pathname = usePathname() - - // Helper function to update specific query parameters const updateQuery = useCallback((updates: Partial) => { - const newQuery = { ...query, ...updates } - newQuery.status = sanitizeStatusValue(newQuery.status) - newQuery.sort = sanitizeSortValue(newQuery.sort) - const params = new URLSearchParams() - updateSearchParams(newQuery, params) - const search = params.toString() - const queryString = search ? `?${search}` : '' - router.push(`${pathname}${queryString}`, { scroll: false }) - }, [query, router, pathname]) + const patch = { ...updates } + if ('page' in patch && patch.page !== undefined) + patch.page = sanitizePageValue(patch.page) + if ('limit' in patch && patch.limit !== undefined) + patch.limit = sanitizeLimitValue(patch.limit) + if ('status' in patch) + patch.status = sanitizeStatusValue(patch.status) + if ('sort' in patch) + patch.sort = sanitizeSortValue(patch.sort) + if ('keyword' in patch && typeof patch.keyword === 'string' && patch.keyword.trim() === '') + patch.keyword = '' + + // If keyword is part of this patch (even with page reset), treat it as a search update: + // use replace to avoid creating a history entry per input-driven change. + if ('keyword' in patch) { + setQuery(patch, { + history: 'replace', + limitUrlUpdates: patch.keyword === '' ? undefined : KEYWORD_URL_UPDATE_THROTTLE, + }) + return + } + + setQuery(patch, { history: 'push' }) + }, [setQuery]) - // Helper function to reset query to defaults const resetQuery = useCallback(() => { - const params = new URLSearchParams() - updateSearchParams(DEFAULT_QUERY, params) - const search = params.toString() - const queryString = search ? `?${search}` : '' - router.push(`${pathname}${queryString}`, { scroll: false }) - }, [router, pathname]) + setQuery(null, { history: 'replace' }) + }, [setQuery]) return useMemo(() => ({ query, @@ -113,5 +101,3 @@ function useDocumentListQueryState() { resetQuery, }), [query, updateQuery, resetQuery]) } - -export default useDocumentListQueryState diff --git a/web/app/components/datasets/documents/hooks/use-documents-page-state.ts b/web/app/components/datasets/documents/hooks/use-documents-page-state.ts index 4fb227f717..36b1e8c760 100644 --- a/web/app/components/datasets/documents/hooks/use-documents-page-state.ts +++ b/web/app/components/datasets/documents/hooks/use-documents-page-state.ts @@ -1,175 +1,63 @@ -import type { DocumentListResponse } from '@/models/datasets' import type { SortType } from '@/service/datasets' -import { useDebounce, useDebounceFn } from 'ahooks' -import { useCallback, useEffect, useMemo, useState } from 'react' +import { useDebounce } from 'ahooks' +import { useCallback, useState } from 'react' import { normalizeStatusForQuery, sanitizeStatusValue } from '../status-filter' -import useDocumentListQueryState from './use-document-list-query-state' +import { useDocumentListQueryState } from './use-document-list-query-state' -/** - * Custom hook to manage documents page state including: - * - Search state (input value, debounced search value) - * - Filter state (status filter, sort value) - * - Pagination state (current page, limit) - * - Selection state (selected document ids) - * - Polling state (timer control for auto-refresh) - */ export function useDocumentsPageState() { const { query, updateQuery } = useDocumentListQueryState() - // Search state - const [inputValue, setInputValue] = useState('') - const [searchValue, setSearchValue] = useState('') - const debouncedSearchValue = useDebounce(searchValue, { wait: 500 }) + const inputValue = query.keyword + const debouncedSearchValue = useDebounce(query.keyword, { wait: 500 }) - // Filter & sort state - const [statusFilterValue, setStatusFilterValue] = useState(() => sanitizeStatusValue(query.status)) - const [sortValue, setSortValue] = useState(query.sort) - const normalizedStatusFilterValue = useMemo( - () => normalizeStatusForQuery(statusFilterValue), - [statusFilterValue], - ) + const statusFilterValue = sanitizeStatusValue(query.status) + const sortValue = query.sort + const normalizedStatusFilterValue = normalizeStatusForQuery(statusFilterValue) - // Pagination state - const [currPage, setCurrPage] = useState(query.page - 1) - const [limit, setLimit] = useState(query.limit) + const currPage = query.page - 1 + const limit = query.limit - // Selection state const [selectedIds, setSelectedIds] = useState([]) - // Polling state - const [timerCanRun, setTimerCanRun] = useState(true) - - // Initialize search value from URL on mount - useEffect(() => { - if (query.keyword) { - setInputValue(query.keyword) - setSearchValue(query.keyword) - } - }, []) // Only run on mount - - // Sync local state with URL query changes - useEffect(() => { - setCurrPage(query.page - 1) - setLimit(query.limit) - if (query.keyword !== searchValue) { - setInputValue(query.keyword) - setSearchValue(query.keyword) - } - setStatusFilterValue((prev) => { - const nextValue = sanitizeStatusValue(query.status) - return prev === nextValue ? prev : nextValue - }) - setSortValue(query.sort) - }, [query]) - - // Update URL when search changes - useEffect(() => { - if (debouncedSearchValue !== query.keyword) { - setCurrPage(0) - updateQuery({ keyword: debouncedSearchValue, page: 1 }) - } - }, [debouncedSearchValue, query.keyword, updateQuery]) - - // Clear selection when search changes - useEffect(() => { - if (searchValue !== query.keyword) - setSelectedIds([]) - }, [searchValue, query.keyword]) - - // Clear selection when status filter changes - useEffect(() => { - setSelectedIds([]) - }, [normalizedStatusFilterValue]) - - // Page change handler const handlePageChange = useCallback((newPage: number) => { - setCurrPage(newPage) updateQuery({ page: newPage + 1 }) }, [updateQuery]) - // Limit change handler const handleLimitChange = useCallback((newLimit: number) => { - setLimit(newLimit) - setCurrPage(0) updateQuery({ limit: newLimit, page: 1 }) }, [updateQuery]) - // Debounced search handler - const { run: handleSearch } = useDebounceFn(() => { - setSearchValue(inputValue) - }, { wait: 500 }) - - // Input change handler const handleInputChange = useCallback((value: string) => { - setInputValue(value) - handleSearch() - }, [handleSearch]) + if (value !== query.keyword) + setSelectedIds([]) + updateQuery({ keyword: value, page: 1 }) + }, [query.keyword, updateQuery]) - // Status filter change handler const handleStatusFilterChange = useCallback((value: string) => { const selectedValue = sanitizeStatusValue(value) - setStatusFilterValue(selectedValue) - setCurrPage(0) + setSelectedIds([]) updateQuery({ status: selectedValue, page: 1 }) }, [updateQuery]) - // Status filter clear handler const handleStatusFilterClear = useCallback(() => { if (statusFilterValue === 'all') return - setStatusFilterValue('all') - setCurrPage(0) + setSelectedIds([]) updateQuery({ status: 'all', page: 1 }) }, [statusFilterValue, updateQuery]) - // Sort change handler const handleSortChange = useCallback((value: string) => { const next = value as SortType if (next === sortValue) return - setSortValue(next) - setCurrPage(0) updateQuery({ sort: next, page: 1 }) }, [sortValue, updateQuery]) - // Update polling state based on documents response - const updatePollingState = useCallback((documentsRes: DocumentListResponse | undefined) => { - if (!documentsRes?.data) - return - - let completedNum = 0 - documentsRes.data.forEach((documentItem) => { - const { indexing_status } = documentItem - const isEmbedded = indexing_status === 'completed' || indexing_status === 'paused' || indexing_status === 'error' - if (isEmbedded) - completedNum++ - }) - - const hasIncompleteDocuments = completedNum !== documentsRes.data.length - const transientStatuses = ['queuing', 'indexing', 'paused'] - const shouldForcePolling = normalizedStatusFilterValue === 'all' - ? false - : transientStatuses.includes(normalizedStatusFilterValue) - setTimerCanRun(shouldForcePolling || hasIncompleteDocuments) - }, [normalizedStatusFilterValue]) - - // Adjust page when total pages change - const adjustPageForTotal = useCallback((documentsRes: DocumentListResponse | undefined) => { - if (!documentsRes) - return - const totalPages = Math.ceil(documentsRes.total / limit) - if (currPage > 0 && currPage + 1 > totalPages) - handlePageChange(totalPages > 0 ? totalPages - 1 : 0) - }, [limit, currPage, handlePageChange]) - return { - // Search state inputValue, - searchValue, debouncedSearchValue, handleInputChange, - // Filter & sort state statusFilterValue, sortValue, normalizedStatusFilterValue, @@ -177,21 +65,12 @@ export function useDocumentsPageState() { handleStatusFilterClear, handleSortChange, - // Pagination state currPage, limit, handlePageChange, handleLimitChange, - // Selection state selectedIds, setSelectedIds, - - // Polling state - timerCanRun, - updatePollingState, - adjustPageForTotal, } } - -export default useDocumentsPageState diff --git a/web/app/components/datasets/documents/index.tsx b/web/app/components/datasets/documents/index.tsx index 676e715f56..764b04227c 100644 --- a/web/app/components/datasets/documents/index.tsx +++ b/web/app/components/datasets/documents/index.tsx @@ -1,7 +1,7 @@ 'use client' import type { FC } from 'react' import { useRouter } from 'next/navigation' -import { useCallback, useEffect } from 'react' +import { useCallback } from 'react' import Loading from '@/app/components/base/loading' import { useDatasetDetailContextWithSelector } from '@/context/dataset-detail' import { useProviderContext } from '@/context/provider-context' @@ -13,12 +13,16 @@ import useEditDocumentMetadata from '../metadata/hooks/use-edit-dataset-metadata import DocumentsHeader from './components/documents-header' import EmptyElement from './components/empty-element' import List from './components/list' -import useDocumentsPageState from './hooks/use-documents-page-state' +import { useDocumentsPageState } from './hooks/use-documents-page-state' type IDocumentsProps = { datasetId: string } +const POLLING_INTERVAL = 2500 +const TERMINAL_INDEXING_STATUSES = new Set(['completed', 'paused', 'error']) +const FORCED_POLLING_STATUSES = new Set(['queuing', 'indexing', 'paused']) + const Documents: FC = ({ datasetId }) => { const router = useRouter() const { plan } = useProviderContext() @@ -44,9 +48,6 @@ const Documents: FC = ({ datasetId }) => { handleLimitChange, selectedIds, setSelectedIds, - timerCanRun, - updatePollingState, - adjustPageForTotal, } = useDocumentsPageState() // Fetch document list @@ -59,19 +60,17 @@ const Documents: FC = ({ datasetId }) => { status: normalizedStatusFilterValue, sort: sortValue, }, - refetchInterval: timerCanRun ? 2500 : 0, + refetchInterval: (query) => { + const shouldForcePolling = normalizedStatusFilterValue !== 'all' + && FORCED_POLLING_STATUSES.has(normalizedStatusFilterValue) + const documents = query.state.data?.data + if (!documents) + return POLLING_INTERVAL + const hasIncompleteDocuments = documents.some(({ indexing_status }) => !TERMINAL_INDEXING_STATUSES.has(indexing_status)) + return shouldForcePolling || hasIncompleteDocuments ? POLLING_INTERVAL : false + }, }) - // Update polling state when documents change - useEffect(() => { - updatePollingState(documentsRes) - }, [documentsRes, updatePollingState]) - - // Adjust page when total changes - useEffect(() => { - adjustPageForTotal(documentsRes) - }, [documentsRes, adjustPageForTotal]) - // Invalidation hooks const invalidDocumentList = useInvalidDocumentList(datasetId) const invalidDocumentDetail = useInvalidDocumentDetail() @@ -119,7 +118,7 @@ const Documents: FC = ({ datasetId }) => { // Render content based on loading and data state const renderContent = () => { - if (isListLoading) + if (isListLoading && !documentsRes) return if (total > 0) { @@ -131,8 +130,8 @@ const Documents: FC = ({ datasetId }) => { onUpdate={handleUpdate} selectedIds={selectedIds} onSelectedIdChange={setSelectedIds} - statusFilterValue={normalizedStatusFilterValue} remoteSortValue={sortValue} + onSortChange={handleSortChange} pagination={{ total, limit, diff --git a/web/app/components/explore/app-list/__tests__/index.spec.tsx b/web/app/components/explore/app-list/__tests__/index.spec.tsx index 1a389e21ba..5d7dffd40a 100644 --- a/web/app/components/explore/app-list/__tests__/index.spec.tsx +++ b/web/app/components/explore/app-list/__tests__/index.spec.tsx @@ -1,12 +1,12 @@ import type { Mock } from 'vitest' import type { CreateAppModalProps } from '@/app/components/explore/create-app-modal' import type { App } from '@/models/explore' -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { act, fireEvent, screen, waitFor } from '@testing-library/react' import { useAppContext } from '@/context/app-context' import { useGlobalPublicStore } from '@/context/global-public-context' import { fetchAppDetail } from '@/service/explore' import { useMembers } from '@/service/use-common' +import { renderWithNuqs } from '@/test/nuqs-testing' import { AppModeEnum } from '@/types/app' import AppList from '../index' @@ -132,10 +132,9 @@ const mockMemberRole = (hasEditPermission: boolean) => { const renderAppList = (hasEditPermission = false, onSuccess?: () => void, searchParams?: Record) => { mockMemberRole(hasEditPermission) - return render( - - - , + return renderWithNuqs( + , + { searchParams }, ) } diff --git a/web/app/components/plugins/marketplace/__tests__/atoms.spec.tsx b/web/app/components/plugins/marketplace/__tests__/atoms.spec.tsx index 40fc47a9d2..2fae0f90d6 100644 --- a/web/app/components/plugins/marketplace/__tests__/atoms.spec.tsx +++ b/web/app/components/plugins/marketplace/__tests__/atoms.spec.tsx @@ -1,21 +1,20 @@ -import type { UrlUpdateEvent } from 'nuqs/adapters/testing' import type { ReactNode } from 'react' import { act, renderHook } from '@testing-library/react' import { Provider as JotaiProvider } from 'jotai' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createNuqsTestWrapper } from '@/test/nuqs-testing' import { DEFAULT_SORT } from '../constants' const createWrapper = (searchParams = '') => { - const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>() + const { wrapper: NuqsWrapper } = createNuqsTestWrapper({ searchParams }) const wrapper = ({ children }: { children: ReactNode }) => ( - + {children} - + ) - return { wrapper, onUrlUpdate } + return { wrapper } } describe('Marketplace sort atoms', () => { diff --git a/web/app/components/plugins/marketplace/__tests__/plugin-type-switch.spec.tsx b/web/app/components/plugins/marketplace/__tests__/plugin-type-switch.spec.tsx index 6bb075410e..3e5e6a5e0a 100644 --- a/web/app/components/plugins/marketplace/__tests__/plugin-type-switch.spec.tsx +++ b/web/app/components/plugins/marketplace/__tests__/plugin-type-switch.spec.tsx @@ -1,9 +1,8 @@ -import type { UrlUpdateEvent } from 'nuqs/adapters/testing' import type { ReactNode } from 'react' import { fireEvent, render, screen } from '@testing-library/react' import { Provider as JotaiProvider } from 'jotai' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createNuqsTestWrapper } from '@/test/nuqs-testing' import PluginTypeSwitch from '../plugin-type-switch' vi.mock('#i18n', () => ({ @@ -25,15 +24,15 @@ vi.mock('#i18n', () => ({ })) const createWrapper = (searchParams = '') => { - const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>() + const { wrapper: NuqsWrapper } = createNuqsTestWrapper({ searchParams }) const Wrapper = ({ children }: { children: ReactNode }) => ( - + {children} - + ) - return { Wrapper, onUrlUpdate } + return { Wrapper } } describe('PluginTypeSwitch', () => { diff --git a/web/app/components/plugins/marketplace/__tests__/state.spec.tsx b/web/app/components/plugins/marketplace/__tests__/state.spec.tsx index 4177c9b2b7..f4330f31b4 100644 --- a/web/app/components/plugins/marketplace/__tests__/state.spec.tsx +++ b/web/app/components/plugins/marketplace/__tests__/state.spec.tsx @@ -2,8 +2,8 @@ import type { ReactNode } from 'react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { renderHook, waitFor } from '@testing-library/react' import { Provider as JotaiProvider } from 'jotai' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createNuqsTestWrapper } from '@/test/nuqs-testing' vi.mock('@/config', () => ({ API_PREFIX: '/api', @@ -37,6 +37,7 @@ vi.mock('@/service/client', () => ({ })) const createWrapper = (searchParams = '') => { + const { wrapper: NuqsWrapper } = createNuqsTestWrapper({ searchParams }) const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: 0 }, @@ -45,9 +46,9 @@ const createWrapper = (searchParams = '') => { const Wrapper = ({ children }: { children: ReactNode }) => ( - + {children} - + ) diff --git a/web/app/components/plugins/marketplace/__tests__/sticky-search-and-switch-wrapper.spec.tsx b/web/app/components/plugins/marketplace/__tests__/sticky-search-and-switch-wrapper.spec.tsx index 1311adb508..1876692dad 100644 --- a/web/app/components/plugins/marketplace/__tests__/sticky-search-and-switch-wrapper.spec.tsx +++ b/web/app/components/plugins/marketplace/__tests__/sticky-search-and-switch-wrapper.spec.tsx @@ -1,8 +1,8 @@ import type { ReactNode } from 'react' import { render } from '@testing-library/react' import { Provider as JotaiProvider } from 'jotai' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createNuqsTestWrapper } from '@/test/nuqs-testing' import StickySearchAndSwitchWrapper from '../sticky-search-and-switch-wrapper' vi.mock('#i18n', () => ({ @@ -20,13 +20,17 @@ vi.mock('../search-box/search-box-wrapper', () => ({ default: () =>
SearchBoxWrapper
, })) -const Wrapper = ({ children }: { children: ReactNode }) => ( - - - {children} - - -) +const createWrapper = () => { + const { wrapper: NuqsWrapper } = createNuqsTestWrapper() + const Wrapper = ({ children }: { children: ReactNode }) => ( + + + {children} + + + ) + return { Wrapper } +} describe('StickySearchAndSwitchWrapper', () => { beforeEach(() => { @@ -34,6 +38,7 @@ describe('StickySearchAndSwitchWrapper', () => { }) it('should render SearchBoxWrapper and PluginTypeSwitch', () => { + const { Wrapper } = createWrapper() const { getByTestId } = render( , { wrapper: Wrapper }, @@ -44,6 +49,7 @@ describe('StickySearchAndSwitchWrapper', () => { }) it('should not apply sticky class when no pluginTypeSwitchClassName', () => { + const { Wrapper } = createWrapper() const { container } = render( , { wrapper: Wrapper }, @@ -55,6 +61,7 @@ describe('StickySearchAndSwitchWrapper', () => { }) it('should apply sticky class when pluginTypeSwitchClassName contains top-', () => { + const { Wrapper } = createWrapper() const { container } = render( , { wrapper: Wrapper }, @@ -67,6 +74,7 @@ describe('StickySearchAndSwitchWrapper', () => { }) it('should not apply sticky class when pluginTypeSwitchClassName does not contain top-', () => { + const { Wrapper } = createWrapper() const { container } = render( , { wrapper: Wrapper }, diff --git a/web/app/components/plugins/marketplace/hydration-server.tsx b/web/app/components/plugins/marketplace/hydration-server.tsx index 287bcbe8c0..2b1dea25cb 100644 --- a/web/app/components/plugins/marketplace/hydration-server.tsx +++ b/web/app/components/plugins/marketplace/hydration-server.tsx @@ -1,4 +1,5 @@ import type { SearchParams } from 'nuqs/server' +import type { MarketplaceSearchParams } from './search-params' import { dehydrate, HydrationBoundary } from '@tanstack/react-query' import { createLoader } from 'nuqs/server' import { getQueryClientServer } from '@/context/query-client-server' @@ -14,7 +15,7 @@ async function getDehydratedState(searchParams?: Promise) { return } const loadSearchParams = createLoader(marketplaceSearchParamsParsers) - const params = await loadSearchParams(searchParams) + const params: MarketplaceSearchParams = await loadSearchParams(searchParams) if (!PLUGIN_CATEGORY_WITH_COLLECTIONS.has(params.category)) { return diff --git a/web/app/components/plugins/marketplace/search-params.ts b/web/app/components/plugins/marketplace/search-params.ts index ad0b16977f..a7da306045 100644 --- a/web/app/components/plugins/marketplace/search-params.ts +++ b/web/app/components/plugins/marketplace/search-params.ts @@ -1,3 +1,4 @@ +import type { inferParserType } from 'nuqs/server' import type { ActivePluginType } from './constants' import { parseAsArrayOf, parseAsString, parseAsStringEnum } from 'nuqs/server' import { PLUGIN_TYPE_SEARCH_MAP } from './constants' @@ -7,3 +8,5 @@ export const marketplaceSearchParamsParsers = { q: parseAsString.withDefault('').withOptions({ history: 'replace' }), tags: parseAsArrayOf(parseAsString).withDefault([]).withOptions({ history: 'replace' }), } + +export type MarketplaceSearchParams = inferParserType diff --git a/web/app/components/plugins/plugin-page/__tests__/context.spec.tsx b/web/app/components/plugins/plugin-page/__tests__/context.spec.tsx index 4dd23f53f1..ab0e35e042 100644 --- a/web/app/components/plugins/plugin-page/__tests__/context.spec.tsx +++ b/web/app/components/plugins/plugin-page/__tests__/context.spec.tsx @@ -7,6 +7,9 @@ import { PluginPageContext, PluginPageContextProvider, usePluginPageContext } fr // Mock dependencies vi.mock('nuqs', () => ({ + parseAsStringEnum: vi.fn(() => ({ + withDefault: vi.fn(() => ({})), + })), useQueryState: vi.fn(() => ['plugins', vi.fn()]), })) diff --git a/web/app/components/plugins/plugin-page/__tests__/index.spec.tsx b/web/app/components/plugins/plugin-page/__tests__/index.spec.tsx index be9f0b1858..dafcbe57c2 100644 --- a/web/app/components/plugins/plugin-page/__tests__/index.spec.tsx +++ b/web/app/components/plugins/plugin-page/__tests__/index.spec.tsx @@ -80,6 +80,9 @@ vi.mock('@/service/use-plugins', () => ({ })) vi.mock('nuqs', () => ({ + parseAsStringEnum: vi.fn(() => ({ + withDefault: vi.fn(() => ({})), + })), useQueryState: vi.fn(() => ['plugins', vi.fn()]), })) diff --git a/web/app/components/plugins/plugin-page/context.tsx b/web/app/components/plugins/plugin-page/context.tsx index abc4408d62..01ec518347 100644 --- a/web/app/components/plugins/plugin-page/context.tsx +++ b/web/app/components/plugins/plugin-page/context.tsx @@ -3,7 +3,7 @@ import type { ReactNode, RefObject } from 'react' import type { FilterState } from './filter-management' import { noop } from 'es-toolkit/function' -import { useQueryState } from 'nuqs' +import { parseAsStringEnum, useQueryState } from 'nuqs' import { useMemo, useRef, @@ -15,6 +15,19 @@ import { } from 'use-context-selector' import { useGlobalPublicStore } from '@/context/global-public-context' import { PLUGIN_PAGE_TABS_MAP, usePluginPageTabs } from '../hooks' +import { PLUGIN_TYPE_SEARCH_MAP } from '../marketplace/constants' + +export type PluginPageTab = typeof PLUGIN_PAGE_TABS_MAP[keyof typeof PLUGIN_PAGE_TABS_MAP] + | (typeof PLUGIN_TYPE_SEARCH_MAP)[keyof typeof PLUGIN_TYPE_SEARCH_MAP] + +const PLUGIN_PAGE_TAB_VALUES: PluginPageTab[] = [ + PLUGIN_PAGE_TABS_MAP.plugins, + PLUGIN_PAGE_TABS_MAP.marketplace, + ...Object.values(PLUGIN_TYPE_SEARCH_MAP), +] + +const parseAsPluginPageTab = parseAsStringEnum(PLUGIN_PAGE_TAB_VALUES) + .withDefault(PLUGIN_PAGE_TABS_MAP.plugins) export type PluginPageContextValue = { containerRef: RefObject @@ -22,8 +35,8 @@ export type PluginPageContextValue = { setCurrentPluginID: (pluginID?: string) => void filters: FilterState setFilters: (filter: FilterState) => void - activeTab: string - setActiveTab: (tab: string) => void + activeTab: PluginPageTab + setActiveTab: (tab: PluginPageTab) => void options: Array<{ value: string, text: string }> } @@ -39,7 +52,7 @@ export const PluginPageContext = createContext({ searchQuery: '', }, setFilters: noop, - activeTab: '', + activeTab: PLUGIN_PAGE_TABS_MAP.plugins, setActiveTab: noop, options: [], }) @@ -68,9 +81,7 @@ export const PluginPageContextProvider = ({ const options = useMemo(() => { return enable_marketplace ? tabs : tabs.filter(tab => tab.value !== PLUGIN_PAGE_TABS_MAP.marketplace) }, [tabs, enable_marketplace]) - const [activeTab, setActiveTab] = useQueryState('tab', { - defaultValue: options[0].value, - }) + const [activeTab, setActiveTab] = useQueryState('tab', parseAsPluginPageTab) return ( ([ + PLUGIN_PAGE_TABS_MAP.plugins, + PLUGIN_PAGE_TABS_MAP.marketplace, + ...Object.values(PLUGIN_TYPE_SEARCH_MAP), +]) + +const isPluginPageTab = (value: string): value is PluginPageTab => { + return pluginPageTabSet.has(value) +} + export type PluginPageProps = { plugins: React.ReactNode marketplace: React.ReactNode @@ -154,7 +165,10 @@ const PluginPage = ({
{ + if (isPluginPageTab(nextTab)) + setActiveTab(nextTab) + }} options={options} />
diff --git a/web/app/components/tools/__tests__/provider-list.spec.tsx b/web/app/components/tools/__tests__/provider-list.spec.tsx index 2c75c20979..a5cb4821bb 100644 --- a/web/app/components/tools/__tests__/provider-list.spec.tsx +++ b/web/app/components/tools/__tests__/provider-list.spec.tsx @@ -1,6 +1,6 @@ -import { cleanup, fireEvent, render, screen } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { cleanup, fireEvent, screen } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { renderWithNuqs } from '@/test/nuqs-testing' import { ToolTypeEnum } from '../../workflow/block-selector/types' import ProviderList from '../provider-list' import { getToolType } from '../utils' @@ -206,10 +206,9 @@ describe('getToolType', () => { }) const renderProviderList = (searchParams?: Record) => { - return render( - - - , + return renderWithNuqs( + , + { searchParams }, ) } diff --git a/web/app/components/tools/provider-list.tsx b/web/app/components/tools/provider-list.tsx index ed6136f3c5..3501726cd0 100644 --- a/web/app/components/tools/provider-list.tsx +++ b/web/app/components/tools/provider-list.tsx @@ -1,6 +1,6 @@ 'use client' import type { Collection } from './types' -import { useQueryState } from 'nuqs' +import { parseAsStringLiteral, useQueryState } from 'nuqs' import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { useTranslation } from 'react-i18next' import Input from '@/app/components/base/input' @@ -23,6 +23,17 @@ import { useMarketplace } from './marketplace/hooks' import MCPList from './mcp' import { getToolType } from './utils' +const TOOL_PROVIDER_CATEGORY_VALUES = ['builtin', 'api', 'workflow', 'mcp'] as const +type ToolProviderCategory = typeof TOOL_PROVIDER_CATEGORY_VALUES[number] +const toolProviderCategorySet = new Set(TOOL_PROVIDER_CATEGORY_VALUES) + +const isToolProviderCategory = (value: string): value is ToolProviderCategory => { + return toolProviderCategorySet.has(value) +} + +const parseAsToolProviderCategory = parseAsStringLiteral(TOOL_PROVIDER_CATEGORY_VALUES) + .withDefault('builtin') + const ProviderList = () => { // const searchParams = useSearchParams() // searchParams.get('category') === 'workflow' @@ -31,9 +42,7 @@ const ProviderList = () => { const { enable_marketplace } = useGlobalPublicStore(s => s.systemFeatures) const containerRef = useRef(null) - const [activeTab, setActiveTab] = useQueryState('category', { - defaultValue: 'builtin', - }) + const [activeTab, setActiveTab] = useQueryState('category', parseAsToolProviderCategory) const options = [ { value: 'builtin', text: t('type.builtIn', { ns: 'tools' }) }, { value: 'api', text: t('type.custom', { ns: 'tools' }) }, @@ -124,6 +133,8 @@ const ProviderList = () => { { + if (!isToolProviderCategory(state)) + return setActiveTab(state) if (state !== activeTab) setCurrentProviderId(undefined) diff --git a/web/context/modal-context.test.tsx b/web/context/modal-context.test.tsx index 2f2d09c6f0..a0f6ff35ec 100644 --- a/web/context/modal-context.test.tsx +++ b/web/context/modal-context.test.tsx @@ -1,9 +1,9 @@ -import { act, render, screen, waitFor } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { act, screen, waitFor } from '@testing-library/react' import * as React from 'react' import { defaultPlan } from '@/app/components/billing/config' import { Plan } from '@/app/components/billing/type' import { ModalContextProvider } from '@/context/modal-context' +import { renderWithNuqs } from '@/test/nuqs-testing' vi.mock('@/config', async (importOriginal) => { const actual = await importOriginal() @@ -71,12 +71,10 @@ const createPlan = (overrides: PlanOverrides = {}): PlanShape => ({ }, }) -const renderProvider = () => render( - - -
- - , +const renderProvider = () => renderWithNuqs( + +
+ , ) describe('ModalContextProvider trigger events limit modal', () => { diff --git a/web/context/modal-context.tsx b/web/context/modal-context.tsx index 293970259a..ae1184e5bf 100644 --- a/web/context/modal-context.tsx +++ b/web/context/modal-context.tsx @@ -158,7 +158,7 @@ export const ModalContextProvider = ({ }: ModalContextProviderProps) => { // Use nuqs hooks for URL-based modal state management const [showPricingModal, setPricingModalOpen] = usePricingModal() - const [urlAccountModalState, setUrlAccountModalState] = useAccountSettingModal() + const [urlAccountModalState, setUrlAccountModalState] = useAccountSettingModal() const accountSettingCallbacksRef = useRef, 'payload'> | null>(null) const accountSettingTab = urlAccountModalState.isOpen diff --git a/web/docs/test.md b/web/docs/test.md index cac0e0e351..0204ce0c77 100644 --- a/web/docs/test.md +++ b/web/docs/test.md @@ -225,6 +225,38 @@ Simulate the interactions that matter to users—primary clicks, change events, Mock the specific Next.js navigation hooks your component consumes (`useRouter`, `usePathname`, `useSearchParams`) and drive realistic routing flows—query parameters, redirects, guarded routes, URL updates—while asserting the rendered outcome or navigation side effects. +#### 7.1 `nuqs` Query State Testing + +When testing code that uses `useQueryState` or `useQueryStates`, treat `nuqs` as the source of truth for URL synchronization. + +- ✅ In runtime, keep `NuqsAdapter` in app layout (already wired in `app/layout.tsx`). +- ✅ In tests, wrap with `NuqsTestingAdapter` (prefer helper utilities from `@/test/nuqs-testing`). +- ✅ Assert URL behavior via `onUrlUpdate` events (`searchParams`, `options.history`) instead of only asserting router mocks. +- ✅ For custom parsers created with `createParser`, keep `parse` and `serialize` bijective (round-trip safe). Add edge-case coverage for values like `%2F`, `%25`, spaces, and legacy encoded URLs. +- ✅ Assert default-clearing behavior explicitly (`clearOnDefault` semantics remove params when value equals default). +- ⚠️ Only mock `nuqs` directly when URL behavior is intentionally out of scope for the test. For ESM-safe partial mocks, use async `vi.mock` with `importOriginal`. + +Example: + +```tsx +import { renderHookWithNuqs } from '@/test/nuqs-testing' + +it('should update query with push history', async () => { + const { result, onUrlUpdate } = renderHookWithNuqs(() => useMyQueryState(), { + searchParams: '?page=1', + }) + + act(() => { + result.current.setQuery({ page: 2 }) + }) + + await waitFor(() => expect(onUrlUpdate).toHaveBeenCalled()) + const update = onUrlUpdate.mock.calls.at(-1)![0] + expect(update.options.history).toBe('push') + expect(update.searchParams.get('page')).toBe('2') +}) +``` + ### 8. Edge Cases (REQUIRED - All Components) **Must Test**: diff --git a/web/eslint-suppressions.json b/web/eslint-suppressions.json index 95f00e92b3..4b64291612 100644 --- a/web/eslint-suppressions.json +++ b/web/eslint-suppressions.json @@ -3404,11 +3404,6 @@ "count": 1 } }, - "app/components/datasets/documents/components/list.tsx": { - "react-refresh/only-export-components": { - "count": 1 - } - }, "app/components/datasets/documents/components/operations.tsx": { "no-restricted-imports": { "count": 1 @@ -3853,16 +3848,6 @@ "count": 3 } }, - "app/components/datasets/documents/hooks/use-documents-page-state.ts": { - "react-hooks-extra/no-direct-set-state-in-use-effect": { - "count": 12 - } - }, - "app/components/datasets/documents/index.tsx": { - "react-hooks-extra/no-direct-set-state-in-use-effect": { - "count": 2 - } - }, "app/components/datasets/documents/status-item/index.tsx": { "no-restricted-imports": { "count": 1 diff --git a/web/hooks/use-query-params.spec.tsx b/web/hooks/use-query-params.spec.tsx index 35e234881d..ef744742cf 100644 --- a/web/hooks/use-query-params.spec.tsx +++ b/web/hooks/use-query-params.spec.tsx @@ -1,8 +1,6 @@ -import type { UrlUpdateEvent } from 'nuqs/adapters/testing' -import type { ReactNode } from 'react' -import { act, renderHook, waitFor } from '@testing-library/react' -import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { act, waitFor } from '@testing-library/react' import { ACCOUNT_SETTING_MODAL_ACTION } from '@/app/components/header/account-setting/constants' +import { renderHookWithNuqs } from '@/test/nuqs-testing' import { clearQueryParams, PRICING_MODAL_QUERY_PARAM, @@ -20,14 +18,7 @@ vi.mock('@/utils/client', () => ({ })) const renderWithAdapter = (hook: () => T, searchParams = '') => { - const onUrlUpdate = vi.fn<(event: UrlUpdateEvent) => void>() - const wrapper = ({ children }: { children: ReactNode }) => ( - - {children} - - ) - const { result } = renderHook(hook, { wrapper }) - return { result, onUrlUpdate } + return renderHookWithNuqs(hook, { searchParams }) } // Query param hooks: defaults, parsing, and URL sync behavior. diff --git a/web/hooks/use-query-params.ts b/web/hooks/use-query-params.ts index 0749a1ffa5..2de6cfcd2e 100644 --- a/web/hooks/use-query-params.ts +++ b/web/hooks/use-query-params.ts @@ -13,14 +13,19 @@ * - Use shallow routing to avoid unnecessary re-renders */ +import type { AccountSettingTab } from '@/app/components/header/account-setting/constants' import { createParser, - parseAsString, + parseAsStringEnum, + parseAsStringLiteral, useQueryState, useQueryStates, } from 'nuqs' import { useCallback } from 'react' -import { ACCOUNT_SETTING_MODAL_ACTION } from '@/app/components/header/account-setting/constants' +import { + ACCOUNT_SETTING_MODAL_ACTION, + ACCOUNT_SETTING_TAB, +} from '@/app/components/header/account-setting/constants' import { isServer } from '@/utils/client' /** @@ -52,6 +57,10 @@ export function usePricingModal() { ) } +const accountSettingTabValues = Object.values(ACCOUNT_SETTING_TAB) as AccountSettingTab[] +const parseAsAccountSettingAction = parseAsStringLiteral([ACCOUNT_SETTING_MODAL_ACTION] as const) +const parseAsAccountSettingTab = parseAsStringEnum(accountSettingTabValues) + /** * Hook to manage account setting modal state via URL * @returns [state, setState] - Object with isOpen + payload (tab) and setter @@ -61,11 +70,11 @@ export function usePricingModal() { * setAccountModalState({ payload: 'billing' }) // Sets ?action=showSettings&tab=billing * setAccountModalState(null) // Removes both params */ -export function useAccountSettingModal() { +export function useAccountSettingModal() { const [accountState, setAccountState] = useQueryStates( { - action: parseAsString, - tab: parseAsString, + action: parseAsAccountSettingAction, + tab: parseAsAccountSettingTab, }, { history: 'replace', @@ -73,7 +82,7 @@ export function useAccountSettingModal() { ) const setState = useCallback( - (state: { payload: T } | null) => { + (state: { payload: AccountSettingTab } | null) => { if (!state) { setAccountState({ action: null, tab: null }, { history: 'replace' }) return @@ -88,7 +97,7 @@ export function useAccountSettingModal() { ) const isOpen = accountState.action === ACCOUNT_SETTING_MODAL_ACTION - const currentTab = (isOpen ? accountState.tab : null) as T | null + const currentTab = isOpen ? accountState.tab : null return [{ isOpen, payload: currentTab }, setState] as const } diff --git a/web/service/knowledge/use-document.ts b/web/service/knowledge/use-document.ts index 74c9a77bcf..4eb2b7d282 100644 --- a/web/service/knowledge/use-document.ts +++ b/web/service/knowledge/use-document.ts @@ -1,7 +1,9 @@ +import type { UseQueryOptions } from '@tanstack/react-query' import type { DocumentDownloadResponse, DocumentDownloadZipRequest, MetadataType, SortType } from '../datasets' import type { CommonResponse } from '@/models/common' import type { DocumentDetailResponse, DocumentListResponse, UpdateDocumentBatchParams } from '@/models/datasets' import { + keepPreviousData, useMutation, useQuery, } from '@tanstack/react-query' @@ -14,6 +16,8 @@ import { useInvalid } from '../use-base' const NAME_SPACE = 'knowledge/document' export const useDocumentListKey = [NAME_SPACE, 'documentList'] +type DocumentListRefetchInterval = UseQueryOptions['refetchInterval'] + export const useDocumentList = (payload: { datasetId: string query: { @@ -23,7 +27,7 @@ export const useDocumentList = (payload: { sort?: SortType status?: string } - refetchInterval?: number | false + refetchInterval?: DocumentListRefetchInterval }) => { const { query, datasetId, refetchInterval } = payload const { keyword, page, limit, sort, status } = query @@ -42,6 +46,7 @@ export const useDocumentList = (payload: { queryFn: () => get(`/datasets/${datasetId}/documents`, { params, }), + placeholderData: keepPreviousData, refetchInterval, }) } diff --git a/web/test/nuqs-testing.tsx b/web/test/nuqs-testing.tsx new file mode 100644 index 0000000000..b5bf9fa83c --- /dev/null +++ b/web/test/nuqs-testing.tsx @@ -0,0 +1,60 @@ +import type { UrlUpdateEvent } from 'nuqs/adapters/testing' +import type { ComponentProps, ReactElement, ReactNode } from 'react' +import type { Mock } from 'vitest' +import { render, renderHook } from '@testing-library/react' +import { NuqsTestingAdapter } from 'nuqs/adapters/testing' +import { vi } from 'vitest' + +type NuqsSearchParams = ComponentProps['searchParams'] +type NuqsOnUrlUpdate = (event: UrlUpdateEvent) => void +type NuqsOnUrlUpdateSpy = Mock + +type NuqsTestOptions = { + searchParams?: NuqsSearchParams + onUrlUpdate?: NuqsOnUrlUpdateSpy +} + +type NuqsHookTestOptions = NuqsTestOptions & { + initialProps?: Props +} + +type NuqsWrapperProps = { + children: ReactNode +} + +export const createNuqsTestWrapper = (options: NuqsTestOptions = {}) => { + const { searchParams = '', onUrlUpdate } = options + const urlUpdateSpy = onUrlUpdate ?? vi.fn() + const wrapper = ({ children }: NuqsWrapperProps) => ( + + {children} + + ) + + return { + wrapper, + onUrlUpdate: urlUpdateSpy, + } +} + +export const renderWithNuqs = (ui: ReactElement, options: NuqsTestOptions = {}) => { + const { wrapper, onUrlUpdate } = createNuqsTestWrapper(options) + const rendered = render(ui, { wrapper }) + return { + ...rendered, + onUrlUpdate, + } +} + +export const renderHookWithNuqs = ( + callback: (props: Props) => Result, + options: NuqsHookTestOptions = {}, +) => { + const { initialProps, ...nuqsOptions } = options + const { wrapper, onUrlUpdate } = createNuqsTestWrapper(nuqsOptions) + const rendered = renderHook(callback, { wrapper, initialProps }) + return { + ...rendered, + onUrlUpdate, + } +}