mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-26 04:56:36 +00:00
fix(flows): stop the pager bouncing the user back to the flows list
Stepping to a sibling flow landed on /flows. The detail page inferred "this flow does not exist" from three absences — not loading, no flow, no error — and an Apollo variables change satisfies all three: it reports networkStatus setVariables, not loading, while the new flow's data is still undefined. Instrumenting history showed both hops: replaceState to /flows/2861, then straight back out to /flows. The provider now publishes a positive isFlowMissing (the query settled with no flow, or failed as a not-found) and the redirect reads only that, so retuning the loading flag cannot silently break navigation again — which is how this shipped. isLoading itself becomes "in flight with nothing to show", which also keeps the Retry button on a failed load from ejecting the user. Nothing in the suite pressed Prev or Next, so the new spec does: it samples the DOM through a delayed fetch, proving the URL never passes through the list and the pager stays mounted while the sibling loads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
3d04fcfff2
commit
5b4ded1e3c
@@ -0,0 +1,75 @@
|
||||
import { expect, test } from '../../fixtures/test.ts';
|
||||
import { expectCleanPage } from '../../helpers/errors.ts';
|
||||
import { flowsCassette, flowTabsCassette } from '../../mocks/cassettes/flows.ts';
|
||||
|
||||
// Nothing else in the suite presses Prev/Next, which is how a redirect that fired mid-switch
|
||||
// shipped: every other flow spec reaches a detail page through the list.
|
||||
test.describe('flow pager', { tag: ['@flows', '@smoke'] }, () => {
|
||||
test.use({ cassette: flowsCassette() });
|
||||
|
||||
test.describe('with a report to show', () => {
|
||||
test.use({ cassette: flowTabsCassette() });
|
||||
|
||||
// Not covered by the pixel baselines: the header cluster is well under the visual
|
||||
// project's maxDiffPixelRatio, so a reordering passes there unnoticed.
|
||||
test('keeps the variable action left of the fixed ones', async ({ page }) => {
|
||||
await page.goto('/flows/5');
|
||||
await expect(page.locator('header').getByRole('button', { name: 'Report' })).toBeVisible();
|
||||
|
||||
const labels = await page
|
||||
.locator('header button')
|
||||
.evaluateAll((buttons) => buttons.map((button) => button.getAttribute('aria-label') ?? ''));
|
||||
const positionOf = (label: string) => labels.findIndex((candidate) => candidate.startsWith(label));
|
||||
|
||||
// Report comes and goes with the task list; anchored leftmost, its arrival cannot shift
|
||||
// the controls that are always there.
|
||||
expect(positionOf('Report')).toBeLessThan(positionOf('Toggle favorite'));
|
||||
expect(positionOf('Toggle favorite')).toBeLessThan(positionOf('Previous'));
|
||||
expect(positionOf('Next')).toBeLessThan(positionOf('Flow actions'));
|
||||
});
|
||||
});
|
||||
|
||||
test('steps to the sibling flow and back without passing through the list', async ({ page, pageErrorLog }) => {
|
||||
const header = page.locator('header');
|
||||
|
||||
await page.goto('/flows/5');
|
||||
await expect(header.getByText('E2E Alpha')).toBeVisible();
|
||||
|
||||
// Hold the sibling's data back so the in-flight window is wide enough to sample: the
|
||||
// cassette answers in single-digit milliseconds, which hides an unmount entirely.
|
||||
await page.route('**/graphql', async (route) => {
|
||||
await new Promise((resolve) => setTimeout(resolve, 300));
|
||||
await route.fallback();
|
||||
});
|
||||
|
||||
await header.getByRole('button', { name: 'Next' }).click();
|
||||
|
||||
const samples = await page.evaluate(async () => {
|
||||
const taken = [];
|
||||
|
||||
for (let index = 0; index < 10; index += 1) {
|
||||
taken.push({
|
||||
hasPager: !!document.querySelector('header button[aria-label="Next"]'),
|
||||
path: window.location.pathname,
|
||||
});
|
||||
await new Promise((resolve) => setTimeout(resolve, 30));
|
||||
}
|
||||
|
||||
return taken;
|
||||
});
|
||||
|
||||
await expect(page).toHaveURL(/\/flows\/6$/);
|
||||
await expect(header.getByText('E2E Beta')).toBeVisible();
|
||||
|
||||
// Sampled rather than awaited: `toBeVisible` retries, so it passes even if the cluster
|
||||
// unmounts for the length of the fetch and comes back — which is what gating the pager
|
||||
// on the loaded flow did, costing the user a round trip per step.
|
||||
expect(samples.every((sample) => sample.hasPager)).toBe(true);
|
||||
expect(samples.map((sample) => sample.path)).not.toContain('/flows');
|
||||
|
||||
await header.getByRole('button', { name: 'Previous' }).click();
|
||||
|
||||
await expect(page).toHaveURL(/\/flows\/5$/);
|
||||
expectCleanPage(pageErrorLog);
|
||||
});
|
||||
});
|
||||
@@ -80,7 +80,7 @@ function Flow() {
|
||||
const { isDesktop, isMobile } = useBreakpoint();
|
||||
const navigate = useNavigate();
|
||||
|
||||
const { flowData, flowId, flowLoadError, isLoading: isFlowLoading, refetchFlow } = useFlow();
|
||||
const { flowData, flowId, flowLoadError, isFlowMissing, isLoading: isFlowLoading, refetchFlow } = useFlow();
|
||||
const { deleteFlow, finishFlow } = useFlows();
|
||||
const { isFavoriteFlow, toggleFavoriteFlow } = useFavorites();
|
||||
|
||||
@@ -109,13 +109,10 @@ function Flow() {
|
||||
const [renameFlowMutation, { loading: isRenameLoading }] = useMutation(RenameFlowDocument);
|
||||
|
||||
useEffect(() => {
|
||||
// Redirect only when the flow is genuinely gone (query resolved with no flow, or
|
||||
// a not-found error). A real load failure keeps the user here behind the
|
||||
// ErrorState + Retry below instead of silently bouncing to the list.
|
||||
if (!isFlowLoading && !flowData?.flow && !flowLoadError) {
|
||||
if (isFlowMissing) {
|
||||
navigate(routes.flows, { replace: true });
|
||||
}
|
||||
}, [flowData, flowLoadError, isFlowLoading, navigate]);
|
||||
}, [isFlowMissing, navigate]);
|
||||
|
||||
const handleFlowRenameSave = useCallback(async () => {
|
||||
const newTitle = editingInputRef.current?.value.trim();
|
||||
@@ -278,24 +275,14 @@ function Flow() {
|
||||
</BreadcrumbList>
|
||||
</Breadcrumb>
|
||||
</AppHeaderContent>
|
||||
<AppHeaderActions
|
||||
pager={
|
||||
flow &&
|
||||
!isMobile && (
|
||||
<DetailNavigationToolbar<FlowItem>
|
||||
controller={flowNav}
|
||||
renderItem={renderFlowItem}
|
||||
sheetIcon={<GitFork className="size-4" />}
|
||||
sheetTitle="Flows"
|
||||
/>
|
||||
)
|
||||
}
|
||||
>
|
||||
<AppHeaderActions>
|
||||
{!!(flowData?.tasks ?? [])?.length && <FlowReportDropdown />}
|
||||
{flowId && !isMobile && (
|
||||
<Button
|
||||
aria-label="Toggle favorite"
|
||||
aria-pressed={isFavoriteFlow(flowId)}
|
||||
className="shrink-0"
|
||||
disabled={isFlowLoading}
|
||||
onClick={() => toggleFavoriteFlow(flowId)}
|
||||
size="icon"
|
||||
variant="ghost"
|
||||
@@ -303,101 +290,112 @@ function Flow() {
|
||||
<Star className={isFavoriteFlow(flowId) ? 'fill-yellow-500 stroke-yellow-500' : ''} />
|
||||
</Button>
|
||||
)}
|
||||
{!!(flowData?.tasks ?? [])?.length && <FlowReportDropdown />}
|
||||
{flow && (
|
||||
<DropdownMenu>
|
||||
<DropdownMenuTrigger asChild>
|
||||
<Button
|
||||
aria-label="Flow actions"
|
||||
className="size-8 p-0"
|
||||
variant="ghost"
|
||||
>
|
||||
<Ellipsis />
|
||||
</Button>
|
||||
</DropdownMenuTrigger>
|
||||
<DropdownMenuContent
|
||||
align="end"
|
||||
className="min-w-24"
|
||||
onCloseAutoFocus={handleDropdownCloseAutoFocus}
|
||||
{!isMobile && (
|
||||
<DetailNavigationToolbar<FlowItem>
|
||||
controller={flowNav}
|
||||
renderItem={renderFlowItem}
|
||||
sheetIcon={<GitFork className="size-4" />}
|
||||
sheetTitle="Flows"
|
||||
/>
|
||||
)}
|
||||
<DropdownMenu>
|
||||
<DropdownMenuTrigger asChild>
|
||||
<Button
|
||||
aria-label="Flow actions"
|
||||
className="size-8 p-0"
|
||||
variant="ghost"
|
||||
>
|
||||
{isMobile && flowNav.total > 0 && (
|
||||
<>
|
||||
{/* onSelect={preventDefault} stops the Radix menu from closing on label
|
||||
<Ellipsis />
|
||||
</Button>
|
||||
</DropdownMenuTrigger>
|
||||
<DropdownMenuContent
|
||||
align="end"
|
||||
className="min-w-24"
|
||||
onCloseAutoFocus={handleDropdownCloseAutoFocus}
|
||||
>
|
||||
{isMobile && (
|
||||
<>
|
||||
{/* onSelect={preventDefault} stops the Radix menu from closing on label
|
||||
clicks; DetailNavigationButtons owns its own click handlers. */}
|
||||
<DropdownMenuItem
|
||||
className="cursor-default hover:bg-transparent focus:bg-transparent"
|
||||
onSelect={(event) => event.preventDefault()}
|
||||
>
|
||||
<GitFork />
|
||||
Flows
|
||||
<div className="-my-1.5 -mr-2 ml-auto flex items-center">
|
||||
<DetailNavigationButtons<FlowItem>
|
||||
controller={flowNav}
|
||||
sheetTitle="Flows"
|
||||
size="sm"
|
||||
/>
|
||||
</div>
|
||||
</DropdownMenuItem>
|
||||
{flowId && (
|
||||
<DropdownMenuItem onClick={() => toggleFavoriteFlow(flowId)}>
|
||||
<Star
|
||||
className={
|
||||
isFavoriteFlow(flowId)
|
||||
? 'size-4 fill-yellow-500 stroke-yellow-500'
|
||||
: 'size-4'
|
||||
}
|
||||
/>
|
||||
{isFavoriteFlow(flowId) ? 'Remove from favorites' : 'Add to favorites'}
|
||||
</DropdownMenuItem>
|
||||
)}
|
||||
<DropdownMenuSeparator />
|
||||
</>
|
||||
)}
|
||||
<DropdownMenuItem onClick={handleFlowRenameStart}>
|
||||
<PencilLine className="size-3" />
|
||||
Rename
|
||||
</DropdownMenuItem>
|
||||
{isFlowRunning && (
|
||||
<DropdownMenuItem
|
||||
disabled={isFinishing}
|
||||
onClick={() => handleFlowFinish()}
|
||||
className="cursor-default hover:bg-transparent focus:bg-transparent"
|
||||
onSelect={(event) => event.preventDefault()}
|
||||
>
|
||||
{isFinishing ? (
|
||||
<>
|
||||
<Spinner variant="circle" />
|
||||
Finishing...
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<Pause />
|
||||
Finish
|
||||
</>
|
||||
)}
|
||||
<GitFork />
|
||||
Flows
|
||||
<div className="-my-1.5 -mr-2 ml-auto flex items-center">
|
||||
<DetailNavigationButtons<FlowItem>
|
||||
controller={flowNav}
|
||||
sheetTitle="Flows"
|
||||
size="sm"
|
||||
/>
|
||||
</div>
|
||||
</DropdownMenuItem>
|
||||
)}
|
||||
<DropdownMenuSeparator />
|
||||
{flowId && (
|
||||
<DropdownMenuItem
|
||||
disabled={isFlowLoading}
|
||||
onClick={() => toggleFavoriteFlow(flowId)}
|
||||
>
|
||||
<Star
|
||||
className={
|
||||
isFavoriteFlow(flowId)
|
||||
? 'size-4 fill-yellow-500 stroke-yellow-500'
|
||||
: 'size-4'
|
||||
}
|
||||
/>
|
||||
{isFavoriteFlow(flowId) ? 'Remove from favorites' : 'Add to favorites'}
|
||||
</DropdownMenuItem>
|
||||
)}
|
||||
<DropdownMenuSeparator />
|
||||
</>
|
||||
)}
|
||||
<DropdownMenuItem
|
||||
disabled={isFlowLoading}
|
||||
onClick={handleFlowRenameStart}
|
||||
>
|
||||
<PencilLine className="size-3" />
|
||||
Rename
|
||||
</DropdownMenuItem>
|
||||
{isFlowRunning && (
|
||||
<DropdownMenuItem
|
||||
disabled={isDeleting}
|
||||
onClick={() => setIsDeleteDialogOpen(true)}
|
||||
disabled={isFinishing}
|
||||
onClick={() => handleFlowFinish()}
|
||||
>
|
||||
{isDeleting ? (
|
||||
{isFinishing ? (
|
||||
<>
|
||||
<Spinner variant="circle" />
|
||||
Deleting...
|
||||
Finishing...
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<Trash />
|
||||
Delete
|
||||
<Pause />
|
||||
Finish
|
||||
</>
|
||||
)}
|
||||
</DropdownMenuItem>
|
||||
</DropdownMenuContent>
|
||||
</DropdownMenu>
|
||||
)}
|
||||
)}
|
||||
<DropdownMenuSeparator />
|
||||
<DropdownMenuItem
|
||||
disabled={isDeleting || isFlowLoading}
|
||||
onClick={() => setIsDeleteDialogOpen(true)}
|
||||
>
|
||||
{isDeleting ? (
|
||||
<>
|
||||
<Spinner variant="circle" />
|
||||
Deleting...
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<Trash />
|
||||
Delete
|
||||
</>
|
||||
)}
|
||||
</DropdownMenuItem>
|
||||
</DropdownMenuContent>
|
||||
</DropdownMenu>
|
||||
</AppHeaderActions>
|
||||
</AppHeader>
|
||||
{isMobile && flow && (
|
||||
{isMobile && (
|
||||
<DetailNavigationSheet<FlowItem>
|
||||
controller={flowNav}
|
||||
renderItem={renderFlowItem}
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
import { NetworkStatus } from '@apollo/client';
|
||||
import { useMutation, useQuery, useSubscription } from '@apollo/client/react';
|
||||
import { createContext, useCallback, useContext, useEffect, useMemo, useState } from 'react';
|
||||
import { useParams } from 'react-router-dom';
|
||||
@@ -50,6 +49,7 @@ interface FlowContextValue {
|
||||
flowStatus: StatusType | undefined;
|
||||
initiateAssistantCreation: () => void;
|
||||
isAssistantsLoading: boolean;
|
||||
isFlowMissing: boolean;
|
||||
isLoading: boolean;
|
||||
refetchFlow: () => void;
|
||||
selectAssistant: (assistantId: null | string) => void;
|
||||
@@ -75,7 +75,6 @@ export function FlowProvider({ children }: FlowProviderProps) {
|
||||
data: flowData,
|
||||
error: flowError,
|
||||
loading,
|
||||
networkStatus,
|
||||
refetch: refetchFlow,
|
||||
} = useQuery(FlowDocument, {
|
||||
errorPolicy: 'all',
|
||||
@@ -86,17 +85,20 @@ export function FlowProvider({ children }: FlowProviderProps) {
|
||||
variables: { id: flowId ?? '' },
|
||||
});
|
||||
|
||||
// Only the initial load blocks the UI and gates subscriptions. A background
|
||||
// refetch (e.g. the reconnect reconcile) stays at networkStatus `refetch`, so
|
||||
// it must NOT flip isLoading — otherwise it covers the page with the spinner
|
||||
// overlay and tears down the 14 live subscriptions mid-flight.
|
||||
const isLoading = loading && networkStatus === NetworkStatus.loading;
|
||||
// In flight with nothing to show. A background refetch (the reconnect reconcile) still
|
||||
// holds the previous flow, so it must not raise this: it would cover the page with the
|
||||
// spinner overlay and tear down the 14 live subscriptions mid-flight.
|
||||
const isLoading = loading && !flowData?.flow;
|
||||
|
||||
// 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;
|
||||
|
||||
// Absence of data is not absence of the flow — it is also what an in-flight switch to
|
||||
// another flow looks like, so this reads only settled outcomes.
|
||||
const isFlowMissing = Boolean(flowData && !flowData.flow) || Boolean(flowError && isFlowNotFoundError(flowError));
|
||||
|
||||
const { data: assistantsData, loading: isAssistantsLoading } = useQuery(AssistantsDocument, {
|
||||
fetchPolicy: 'cache-first',
|
||||
nextFetchPolicy: 'cache-first',
|
||||
@@ -397,6 +399,7 @@ export function FlowProvider({ children }: FlowProviderProps) {
|
||||
flowStatus,
|
||||
initiateAssistantCreation,
|
||||
isAssistantsLoading,
|
||||
isFlowMissing,
|
||||
isLoading,
|
||||
refetchFlow,
|
||||
selectAssistant,
|
||||
@@ -417,6 +420,7 @@ export function FlowProvider({ children }: FlowProviderProps) {
|
||||
flowStatus,
|
||||
initiateAssistantCreation,
|
||||
isAssistantsLoading,
|
||||
isFlowMissing,
|
||||
isLoading,
|
||||
refetchFlow,
|
||||
selectAssistant,
|
||||
|
||||
Reference in New Issue
Block a user