From 28ab3d2e6163987cf6f574f4d3d453654c8d3459 Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Thu, 23 Jul 2026 00:21:33 +0700 Subject: [PATCH] fix(ui): render a retryable error on a detail load failure, not "not found" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The template and knowledge detail pages dropped `error` from their query and inferred "this record does not exist" from the absence of data. A real load failure — a network drop, a 5xx, a cold-cache backend error on a deep link — therefore rendered "Template not found" / bounced to the list with a toast, offering no way back in short of retyping the URL. Only a genuine 404 should do that; a transient failure should keep the user on the route behind Retry. Both now split the two outcomes the way flow already does: a real error → in-page ErrorState + Retry; a settled-empty result or a not-found error → the existing redirect/not-found card. The `no rows`/`not found` predicate that flow-provider kept privately becomes the shared `lib/errors.ts#isNotFoundError` now that three call sites need it, and flow-provider moves onto it. Proven by a runtime repro, not by reading: knowledge.test.tsx asserts the in-page error + no redirect on a real failure and the redirect on a genuine not-found — reverting the fix drops it to a failure. errors.test.ts pins the predicate's two sides. e2e repros on both detail routes drive it through the production bundle for CI. Co-Authored-By: Claude Opus 4.8 --- frontend/e2e/specs/crud/knowledges.spec.ts | 17 ++++ .../e2e/specs/crud/template-detail.spec.ts | 18 +++++ frontend/src/lib/errors.test.ts | 19 +++++ frontend/src/lib/errors.ts | 7 ++ .../src/pages/knowledges/knowledge.test.tsx | 78 +++++++++++++++++++ frontend/src/pages/knowledges/knowledge.tsx | 39 +++++++++- frontend/src/pages/templates/template.tsx | 34 ++++++-- frontend/src/providers/flow-provider.tsx | 9 +-- 8 files changed, 208 insertions(+), 13 deletions(-) create mode 100644 frontend/src/lib/errors.test.ts create mode 100644 frontend/src/lib/errors.ts create mode 100644 frontend/src/pages/knowledges/knowledge.test.tsx diff --git a/frontend/e2e/specs/crud/knowledges.spec.ts b/frontend/e2e/specs/crud/knowledges.spec.ts index daf0da8e..3336c33f 100644 --- a/frontend/e2e/specs/crud/knowledges.spec.ts +++ b/frontend/e2e/specs/crud/knowledges.spec.ts @@ -122,4 +122,21 @@ test.describe('knowledges crud', { tag: '@crud' }, () => { expectCleanPage(pageErrorLog); }); }); + + // A real load failure on the detail route used to be mislabelled "not found" and bounced to + // the list with no way back in; it must keep the user on the route behind Retry instead. + test.describe('detail load failure', () => { + test.use({ + cassette: knowledgesCassette({ + queries: { knowledgeDocument: [{ errors: [{ message: 'e2e induced load failure' }] }] }, + }), + }); + + test('shows an in-page error with Retry, not a bounce to the list', async ({ page }) => { + await page.goto(`/knowledges/${KNOWLEDGE_DOC.id}`); + + await expect(page.getByRole('button', { name: 'Try again' })).toBeVisible(); + await expect(page).toHaveURL(new RegExp(`/knowledges/${KNOWLEDGE_DOC.id}$`)); + }); + }); }); diff --git a/frontend/e2e/specs/crud/template-detail.spec.ts b/frontend/e2e/specs/crud/template-detail.spec.ts index 39ec9403..85c43bd1 100644 --- a/frontend/e2e/specs/crud/template-detail.spec.ts +++ b/frontend/e2e/specs/crud/template-detail.spec.ts @@ -66,4 +66,22 @@ test.describe('template detail', { tag: '@coverage' }, () => { expect(raw).toContain('E2E-MARK'); expectCleanPage(pageErrorLog); }); + + // A real load failure must offer Retry in place, not the "Template not found" card that a + // genuine 404 shows — the two used to collapse into the same dead-end. + test.describe('load failure', () => { + test.use({ + cassette: templateDetailCassette({ + queries: { flowTemplate: [{ errors: [{ message: 'e2e induced load failure' }] }] }, + }), + }); + + test('shows an in-page error with Retry, not "not found"', async ({ page }) => { + await page.goto(`/templates/${TEMPLATE_DETAIL.id}`); + + await expect(page.getByRole('button', { name: 'Try again' })).toBeVisible(); + await expect(page.getByText('Template not found')).toBeHidden(); + await expect(page).toHaveURL(new RegExp(`/templates/${TEMPLATE_DETAIL.id}$`)); + }); + }); }); diff --git a/frontend/src/lib/errors.test.ts b/frontend/src/lib/errors.test.ts new file mode 100644 index 00000000..5b06ee9e --- /dev/null +++ b/frontend/src/lib/errors.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest'; + +import { isNotFoundError } from './errors'; + +describe('isNotFoundError', () => { + // The two shapes the backend uses for a genuinely missing record — these redirect to the list. + it.each(['no rows in result set', 'flow not found', 'Record Not Found'])('treats %j as not-found', (message) => { + expect(isNotFoundError(new Error(message))).toBe(true); + }); + + // Everything else is a real load failure — the detail page must keep the user behind Retry, + // not bounce them, so these must NOT read as not-found. + it.each(['network error', 'Failed to fetch', 'connection refused', 'internal server error', 'permission denied'])( + 'treats %j as a real failure', + (message) => { + expect(isNotFoundError(new Error(message))).toBe(false); + }, + ); +}); diff --git a/frontend/src/lib/errors.ts b/frontend/src/lib/errors.ts new file mode 100644 index 00000000..89f3231f --- /dev/null +++ b/frontend/src/lib/errors.ts @@ -0,0 +1,7 @@ +/** + * A backend "no rows in result set" / "not found" GraphQL error, as opposed to a real load + * failure (network, 5xx, cold-cache backend error). Detail pages redirect to the list on the + * former and render an in-page ErrorState + Retry on the latter — collapsing the two silently + * bounces the user off a page that a retry would have loaded. + */ +export const isNotFoundError = (error: { message: string }) => /no rows in result set|not found/i.test(error.message); diff --git a/frontend/src/pages/knowledges/knowledge.test.tsx b/frontend/src/pages/knowledges/knowledge.test.tsx new file mode 100644 index 00000000..30bc34d8 --- /dev/null +++ b/frontend/src/pages/knowledges/knowledge.test.tsx @@ -0,0 +1,78 @@ +import { render, screen } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { routes } from '@/lib/routes'; + +const { navigate } = vi.hoisted(() => ({ navigate: vi.fn() })); +const { toastError } = vi.hoisted(() => ({ toastError: vi.fn() })); +const queryResult = vi.hoisted(() => ({ + current: { data: undefined, error: undefined, loading: false, refetch: vi.fn() } as Record, +})); + +vi.mock('@apollo/client/react', () => ({ + skipToken: Symbol('skipToken'), + useQuery: () => queryResult.current, +})); + +vi.mock('react-router-dom', () => ({ + useNavigate: () => navigate, + useParams: () => ({ knowledgeId: '7' }), +})); + +vi.mock('sonner', () => ({ toast: { error: toastError } })); + +vi.mock('@/providers/knowledges-provider', () => ({ + useKnowledges: () => ({ createKnowledge: vi.fn(), updateKnowledge: vi.fn() }), +})); + +// The layout shell and the form drag in the header/editor tree; the branching under test sits +// above them in Knowledge itself, so stub them to a marker. +vi.mock('@/features/knowledges/knowledge-layout', () => ({ + KnowledgeLayout: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); +vi.mock('@/features/knowledges/knowledge-form', async (importOriginal) => ({ + ...(await importOriginal>()), + KnowledgeForm: () =>
, +})); + +const { default: Knowledge } = await import('./knowledge'); + +describe('Knowledge detail load states', () => { + beforeEach(() => { + navigate.mockClear(); + toastError.mockClear(); + }); + + it('renders an in-page error with Retry on a real load failure, without redirecting', () => { + queryResult.current = { data: undefined, error: new Error('network down'), loading: false, refetch: vi.fn() }; + + render(); + + expect(screen.getByText('Error loading knowledge document')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Try again/ })).toBeInTheDocument(); + expect(navigate).not.toHaveBeenCalled(); + }); + + it('redirects to the list on a genuine not-found error', () => { + queryResult.current = { + data: undefined, + error: new Error('no rows in result set'), + loading: false, + refetch: vi.fn(), + }; + + render(); + + expect(screen.queryByText('Error loading knowledge document')).not.toBeInTheDocument(); + expect(toastError).toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(routes.knowledges, { replace: true }); + }); + + it('redirects to the list when the query resolves with no document', () => { + queryResult.current = { data: { knowledgeDocument: null }, error: undefined, loading: false, refetch: vi.fn() }; + + render(); + + expect(navigate).toHaveBeenCalledWith(routes.knowledges, { replace: true }); + }); +}); diff --git a/frontend/src/pages/knowledges/knowledge.tsx b/frontend/src/pages/knowledges/knowledge.tsx index 672015dc..01a2d7a2 100644 --- a/frontend/src/pages/knowledges/knowledge.tsx +++ b/frontend/src/pages/knowledges/knowledge.tsx @@ -3,6 +3,7 @@ import { useCallback, useEffect, useMemo } from 'react'; import { useNavigate, useParams } from 'react-router-dom'; import { toast } from 'sonner'; +import { ErrorState } from '@/components/shared/error-state'; import { Spinner } from '@/components/ui/spinner'; import { type DirtyFlags, @@ -16,6 +17,7 @@ import { } from '@/features/knowledges/knowledge-form'; import { KnowledgeLayout } from '@/features/knowledges/knowledge-layout'; import { KnowledgeDocumentDocument } from '@/graphql/types'; +import { isNotFoundError } from '@/lib/errors'; import { routes } from '@/lib/routes'; import { useKnowledges } from '@/providers/knowledges-provider'; @@ -27,19 +29,33 @@ function Knowledge() { const isNew = knowledgeId === 'new'; const shouldFetch = Boolean(knowledgeId) && !isNew; - const { data, loading: isLoadingKnowledge } = useQuery( + const { + data, + error, + loading: isLoadingKnowledge, + refetch, + } = useQuery( KnowledgeDocumentDocument, shouldFetch && knowledgeId ? { variables: { id: knowledgeId } } : skipToken, ); const knowledge = data?.knowledgeDocument ?? null; + // A real load failure that left nothing to show, as opposed to a genuine not-found: the page + // renders it as an in-page ErrorState + Retry instead of bouncing to the list. Mirrors flow. + const loadError = error && !knowledge && !isNotFoundError(error) ? error : undefined; useEffect(() => { - if (!isNew && !isLoadingKnowledge && !knowledge) { + // Redirect only when the document is genuinely gone (query settled with no document, or a + // not-found error) — never on a transient load failure, which Retry can recover. + if (isNew || isLoadingKnowledge || loadError) { + return; + } + + if (!knowledge) { toast.error('Knowledge document not found'); navigate(routes.knowledges, { replace: true }); } - }, [isNew, isLoadingKnowledge, knowledge, navigate]); + }, [isNew, isLoadingKnowledge, knowledge, loadError, navigate]); const initialValues = useMemo( () => (knowledge ? documentToFormValues(knowledge) : newDocumentDefaults), @@ -73,6 +89,23 @@ function Knowledge() { [createKnowledge, isNew, knowledgeId, updateKnowledge], ); + if (loadError) { + return ( + +
+ refetch()} + title="Error loading knowledge document" + /> +
+ + ); + } + if (!isNew && !knowledge) { return ( + {pageHeader} +
+ refetchTemplate()} + title="Error loading template" + /> +
+
+ ); + } + + if (!isNew && !isLoadingTemplate && !template) { return (
{pageHeader} diff --git a/frontend/src/providers/flow-provider.tsx b/frontend/src/providers/flow-provider.tsx index badfa5ae..0ca9a336 100644 --- a/frontend/src/providers/flow-provider.tsx +++ b/frontend/src/providers/flow-provider.tsx @@ -34,10 +34,9 @@ import { TerminalLogAddedDocument, VectorStoreLogAddedDocument, } from '@/graphql/types'; +import { isNotFoundError } from '@/lib/errors'; import { Log } from '@/lib/log'; -const isFlowNotFoundError = (error: Error) => /no rows in result set|not found/i.test(error.message); - interface FlowContextValue { assistantLogs: Array; assistants: Array; @@ -92,9 +91,9 @@ export function FlowProvider({ children }: FlowProviderProps) { // A real load failure that left nothing to show (cold cache + backend error on a // deep link), as opposed to a genuine not-found. The detail page renders this as an // in-page ErrorState + Retry instead of silently bouncing to the list. - const flowLoadError = flowError && !flowData?.flow && !isFlowNotFoundError(flowError) ? flowError : undefined; + const flowLoadError = flowError && !flowData?.flow && !isNotFoundError(flowError) ? flowError : undefined; - const isFlowMissing = Boolean(flowData && !flowData.flow) || Boolean(flowError && isFlowNotFoundError(flowError)); + const isFlowMissing = Boolean(flowData && !flowData.flow) || Boolean(flowError && isNotFoundError(flowError)); const { data: assistantsData, loading: isAssistantsLoading } = useQuery(AssistantsDocument, { fetchPolicy: 'cache-first', @@ -196,7 +195,7 @@ export function FlowProvider({ children }: FlowProviderProps) { return; } - if (isFlowNotFoundError(flowError)) { + if (isNotFoundError(flowError)) { toast.error('Flow not found', { id: 'flow-load-error' }); }