mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-24 20:16:30 +00:00
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 '<perm>' 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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
3d5fc75f7b
commit
f0860a7858
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 '<perm>' 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);
|
||||
};
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<AssistantLogFragmentFragment>;
|
||||
assistants: Array<AssistantFragmentFragment>;
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user