From f0860a785805fbe5a74bc3a3e4702938dff7bf9e Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Thu, 23 Jul 2026 13:52:02 +0700 Subject: [PATCH] fix(ui): don't treat an authz denial or a partial error as a missing record Two ways a detail page redirected the user off to the list when it should not have, both surfaced by an A/B review of this branch: - isNotFoundError matched /not found/i, but the backend's authz failure "requested permission '' not found" (graph/context.go) also contains "not found". A user who merely lacked a permission was silently bounced to the list instead of seeing the denial. Authz strings now read as real failures. - flow-provider's isFlowMissing dropped the `!flow` guard that its two siblings (flowLoadError and the not-found toast) apply: under errorPolicy:'all' a partial not-found error rides alongside a flow that loaded fine, so the redirect fired on a flow that had rendered correctly. The disjunct is gated on `!flowData?.flow` again, extracted to a pure `deriveFlowMissing` so the regression is unit-tested. errors.test gains the real authz strings (revert the predicate -> red); flow-provider.test covers the partial-error-with-loaded-flow case (revert -> red). Co-Authored-By: Claude Opus 4.8 --- frontend/src/lib/errors.test.ts | 28 +++++++++++------- frontend/src/lib/errors.ts | 11 ++++++- frontend/src/providers/flow-provider.test.ts | 30 ++++++++++++++++++++ frontend/src/providers/flow-provider.tsx | 13 ++++++++- 4 files changed, 70 insertions(+), 12 deletions(-) create mode 100644 frontend/src/providers/flow-provider.test.ts diff --git a/frontend/src/lib/errors.test.ts b/frontend/src/lib/errors.test.ts index 5b06ee9e..34d7d8f8 100644 --- a/frontend/src/lib/errors.test.ts +++ b/frontend/src/lib/errors.test.ts @@ -3,17 +3,25 @@ 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', + it.each(['no rows in result set', 'flow not found', 'template not found: sql: no rows', 'Record Not Found'])( + 'treats %j as not-found', (message) => { - expect(isNotFoundError(new Error(message))).toBe(false); + expect(isNotFoundError(new Error(message))).toBe(true); }, ); + + // The authz strings are real backend messages that also contain "not found" (see errors.ts) — + // they must stay classified as real failures, so do not drop them as odd-looking fixtures. + it.each([ + 'network error', + 'Failed to fetch', + 'connection refused', + 'internal server error', + "requested permission 'flows.read' not found", + 'not authorized to access this token', + 'no permissions granted', + 'privileges are not set', + ])('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 index 89f3231f..8eda68d8 100644 --- a/frontend/src/lib/errors.ts +++ b/frontend/src/lib/errors.ts @@ -4,4 +4,13 @@ * 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); +export const isNotFoundError = (error: { message: string }) => { + // The backend's authz failure "requested permission '' not found" (graph/context.go) also + // contains "not found"; classing it as a missing record would silently redirect a user who only + // lacks access, instead of surfacing the denial. + if (/\b(permission|privilege|unauthori[sz]ed|not authori[sz]ed|forbidden|access denied)\b/i.test(error.message)) { + return false; + } + + return /no rows in result set|not found/i.test(error.message); +}; diff --git a/frontend/src/providers/flow-provider.test.ts b/frontend/src/providers/flow-provider.test.ts new file mode 100644 index 00000000..7651a728 --- /dev/null +++ b/frontend/src/providers/flow-provider.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; + +import { deriveFlowMissing } from './flow-provider'; + +const flow = { flow: { id: '1' } }; +const notFound = new Error('flow not found'); +const partialNotFound = new Error('assistant not found'); // a nested resolver under errorPolicy:'all' + +describe('deriveFlowMissing', () => { + it('is true when the query settled with the flow genuinely absent', () => { + expect(deriveFlowMissing({ flow: null }, undefined)).toBe(true); + }); + + it('is true on a not-found error with no flow loaded', () => { + expect(deriveFlowMissing(undefined, notFound)).toBe(true); + }); + + // Reverting the `!flowData?.flow` gate turns this red — it is the regression guard. + it('is false when a not-found error rides alongside a loaded flow', () => { + expect(deriveFlowMissing(flow, partialNotFound)).toBe(false); + }); + + it('is false on a real load failure (kept behind Retry, not redirected)', () => { + expect(deriveFlowMissing(undefined, new Error('network error'))).toBe(false); + }); + + it('is false while loading (no data, no error yet)', () => { + expect(deriveFlowMissing(undefined, undefined)).toBe(false); + }); +}); diff --git a/frontend/src/providers/flow-provider.tsx b/frontend/src/providers/flow-provider.tsx index 0ca9a336..1a19f24a 100644 --- a/frontend/src/providers/flow-provider.tsx +++ b/frontend/src/providers/flow-provider.tsx @@ -37,6 +37,17 @@ import { import { isNotFoundError } from '@/lib/errors'; import { Log } from '@/lib/log'; +/** + * Under `errorPolicy:'all'` a partial not-found error surfaces alongside a flow that loaded fine, so + * the not-found disjunct gates on `!flowData?.flow` — mirroring `flowLoadError` and the toast below. + * Without the gate that partial error redirects the user off a flow that rendered correctly. + */ +export const deriveFlowMissing = ( + flowData: null | undefined | { flow: unknown }, + flowError: undefined | { message: string }, +): boolean => + Boolean(flowData && !flowData.flow) || Boolean(flowError && !flowData?.flow && isNotFoundError(flowError)); + interface FlowContextValue { assistantLogs: Array; assistants: Array; @@ -93,7 +104,7 @@ export function FlowProvider({ children }: FlowProviderProps) { // in-page ErrorState + Retry instead of silently bouncing to the list. const flowLoadError = flowError && !flowData?.flow && !isNotFoundError(flowError) ? flowError : undefined; - const isFlowMissing = Boolean(flowData && !flowData.flow) || Boolean(flowError && isNotFoundError(flowError)); + const isFlowMissing = deriveFlowMissing(flowData, flowError); const { data: assistantsData, loading: isAssistantsLoading } = useQuery(AssistantsDocument, { fetchPolicy: 'cache-first',