From 86c37b780c05f98af077549bcfafc96055fd8699 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:04:33 +0000 Subject: [PATCH 1/5] =?UTF-8?q?I=20reproduced=20the=20bug=20in=20tests=20a?= =?UTF-8?q?nd=20fixed=20it.=20A=20fresh=20plan=20now=20goes=20Define=20?= =?UTF-8?q?=E2=86=92=20Generating=20=E2=86=92=20Review=20without=20a=20rel?= =?UTF-8?q?oad=20or=20a=20second=20generation.=20I=20haven't=20tried=20the?= =?UTF-8?q?=20fix=20in=20the=20installed=20native=20app,=20and=20the=20ind?= =?UTF-8?q?ependent=20Astra=20review=20and=20real=20native/mobile=20check?= =?UTF-8?q?=20you=20asked=20for=20still=20need=20to=20happen=20before=20me?= =?UTF-8?q?rge.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cause. The bug shows up when Generate is clicked before the autosave delay finishes: 1. Clicking Generate creates the draft in place, so the page starts its own `GET` of that draft. 2. The server takes seconds to reply to `POST /generate`, because it prepares the repository before marking the draft `generating`. The page's `GET` comes back first, with `status: draft`. 3. When `/generate` is acknowledged, `useDraftCreation` navigates to `/studio/:id` and puts the generating draft in the router state. But in `App.tsx` the two studio routes render the same component tree, so `PlanStudioPage` is not remounted. Its `useDraft` still holds the `draft` copy and ignores the router state. 4. Nothing ever tells the page about the new run, so it doesn't subscribe for progress and stays on Define with the prompt. My first Playwright mock answered `/generate` instantly, and the test passed on the broken code. It only failed once the mock held the reply until after the draft read, which is how the real server behaves. Fix: - `useDraftCreation.ts`: after `/generate` is acknowledged and before navigating, it hands the run ID and the generating draft to the page, the same way the existing-draft Generate path already did. Navigation still replaces `/studio/new`, so back/forward is unchanged. - `useDraft.ts`: - It accepts that generating draft even if its first fetch hasn't returned yet. - A fetch started before the run was acknowledged can no longer overwrite it; fetches started afterwards still win. - Reopening a page from history or a reload with a generating snapshot triggers one background refresh, so a finished plan isn't shown as still generating. It's a single fetch, not polling. - `SetupWizard.tsx`, `setupWizardHooks.ts`, `PlanStudioPage.tsx`: pass the draft along with the run ID. - Run-ID checks on live updates are unchanged: an update from a different run still can't complete this one. Tests: - `PlanStudioPage.freshGenerate.test.tsx` (new): uses the real page, routing, `useDraft` and `SetupWizard`, with create, generate and the draft fetch each held and released by the test. It covers web and desktop/hash routing; the stale read arriving before and after the acknowledgement; ignoring an update from another run; then Review. It also checks one draft and one generation, the URL, back/forward, prompt and focus after autosave, and the prompt kept for retry when Generate fails. All four race cases fail on the original code and pass now. - `useDraft.test.tsx`: three new unit tests for the behaviour above. - `e2e/planner-fresh-generate.pw.ts` (new): Playwright at 320px, 390px, 1440px, and the native minimum window (880×620 with the Linux titlebar). It fails on the original code in all four layouts and passes now. These Playwright runs use the web router; the desktop hash routing is only covered by the vitest test. - One existing assertion in `setupWizardHooks.test.tsx` was updated for the extra argument. - Results: - Full UI unit suite: 3053 of 3054 passed. The one failure was that outdated assertion; after updating it, those files and the planner/page suites pass (927 tests). - 58 related planner Playwright tests pass. - `tsc` and `eslint --max-warnings 0` are clean. Preview screenshots of the Generating and Review states at 390px and in the native minimum window are attached. --- propr-ui/e2e/planner-fresh-generate.pw.ts | 119 ++++++++++ .../components/TaskPlanner/SetupWizard.tsx | 4 +- .../TaskPlanner/setupWizardHooks.test.tsx | 2 +- .../TaskPlanner/setupWizardHooks.ts | 4 +- .../TaskPlanner/useDraftCreation.ts | 8 +- propr-ui/src/hooks/useDraft.test.tsx | 53 +++++ propr-ui/src/hooks/useDraft.ts | 38 +++- .../PlanStudioPage.freshGenerate.test.tsx | 205 ++++++++++++++++++ propr-ui/src/pages/PlanStudioPage.tsx | 6 +- 9 files changed, 422 insertions(+), 17 deletions(-) create mode 100644 propr-ui/e2e/planner-fresh-generate.pw.ts create mode 100644 propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx diff --git a/propr-ui/e2e/planner-fresh-generate.pw.ts b/propr-ui/e2e/planner-fresh-generate.pw.ts new file mode 100644 index 000000000..02884d4cd --- /dev/null +++ b/propr-ui/e2e/planner-fresh-generate.pw.ts @@ -0,0 +1,119 @@ +import { expect, test, type Page } from '@playwright/test'; +import { capture, fixture } from './planner-studio.fixture'; +import { installWindowActions } from './native-chrome.fixture'; + +const draftId = 'fresh-plan'; +const prompt = 'Add a health endpoint to the API'; +const task = { id: 'task-1', title: 'Add a GET /health endpoint with a readiness check', body: '## Context\nOne bounded task.', implementation: '' }; + +const layouts = [ + { name: 'phone-320', viewport: { width: 320, height: 640 } }, + { name: 'phone-390', viewport: { width: 390, height: 844 } }, + { name: 'desktop', viewport: { width: 1440, height: 900 } }, + // The installed app's minimum window, with the native shell's titlebar. + { name: 'native-minimum', viewport: { width: 880, height: 620 }, native: true }, +]; + +/** Installs the native bridge the packaged renderer sees, with a Linux frame. */ +async function installNativeShell(page: Page) { + await page.addInitScript(() => { + const profile = { id: 'fresh-plan-profile', name: 'Preview workspace', kind: 'remote' as const, baseUrl: 'https://fixture.example.test' }; + window.__PROPR_DESKTOP__ = { + isDesktop: true, platform: 'linux', + app: { onDeepLink: () => () => undefined }, + profiles: { list: async () => [profile], getActiveId: async () => profile.id, setActiveId: async () => undefined, save: async () => undefined, remove: async () => undefined }, + connection: { probe: async () => ({ status: 'ready' }) }, + authentication: { authenticate: async () => undefined }, + discovery: { supported: false, discover: async () => [] }, + localSetup: { supported: false, setup: async () => profile }, + externalBrowser: { open: async () => undefined }, + } as unknown as typeof window.__PROPR_DESKTOP__; + }); + await installWindowActions(page, { platform: 'linux' }); +} + +/** + * A server for one fresh draft. Like the real one, POST /generate claims the run only after preparing the repository, + * which takes longer than the page's own read of the new draft: that read answers with the pre-generation draft + * before the acknowledgement arrives. Completion is up to the test. + */ +async function routeFreshDraft(page: Page) { + const server = { creations: 0, generations: 0, reads: 0, complete: () => {} }; + const draft: Record = { + draft_id: draftId, repository: 'integry/propr', initial_prompt: '', status: 'draft', attachments: [], chat_history: [], plan_json: [], + created_at: '2026-10-22T22:00:00Z', context_config: { baseBranch: 'main' }, + }; + let answerFirstRead!: () => void; + const firstReadAnswered = new Promise(resolve => { answerFirstRead = resolve; }); + server.complete = () => Object.assign(draft, { status: 'review', plan_json: [task], name: 'Health endpoint', generation_trace: { runId: 'run-1', steps: [] } }); + await page.route('**/api/planner/drafts', route => { + if (route.request().method() !== 'POST') return route.fallback(); + server.creations++; + draft.initial_prompt = route.request().postDataJSON().prompt; + return route.fulfill({ json: draft }); + }); + await page.route(`**/api/planner/drafts/${draftId}`, async route => { + if (route.request().method() === 'PUT') { + Object.assign(draft, route.request().postDataJSON()); + return route.fulfill({ json: draft }); + } + await route.fulfill({ json: draft }); + if (++server.reads === 1) answerFirstRead(); + }); + await page.route(`**/api/planner/drafts/${draftId}/repository-info`, route => route.fulfill({ json: { defaultBranch: 'main', branches: ['main'] } })); + await page.route('**/api/github/repos/integry/propr/branches', route => route.fulfill({ json: { defaultBranch: 'main', branches: ['main'] } })); + await page.route('**/api/planner/preview', route => route.fulfill({ json: { + success: true, stats: { totalTokens: 0, costEstimate: 0, contextLength: 0, fileCount: 0 }, smartSelection: [], warnings: [], + } })); + await page.route('**/api/planner/generate', async route => { + server.generations++; + expect(route.request().postDataJSON().draftId).toBe(draftId); + await Promise.race([firstReadAnswered, new Promise(resolve => setTimeout(resolve, 5_000))]); + Object.assign(draft, { status: 'generating', generation_trace: { runId: 'run-1', steps: [{ name: 'relevance', status: 'in_progress' }] } }); + await route.fulfill({ json: { success: true, runId: 'run-1', status: 'generating', message: 'started' } }); + }); + return server; +} + +for (const layout of layouts) { + test(`a fresh plan generated before autosave shows Generating, then its Review (${layout.name})`, async ({ page }) => { + await page.setViewportSize(layout.viewport); + if (layout.native) await installNativeShell(page); + await fixture(page); + const server = await routeFreshDraft(page); + await page.goto('/plans'); + await page.evaluate(() => { window.history.pushState(null, '', '/studio/new'); window.dispatchEvent(new PopStateEvent('popstate')); }); + + const composer = page.getByPlaceholder(/Describe the feature/); + await expect(page.getByTestId('branch-chip')).toHaveAttribute('data-status', 'resolved'); + await composer.fill(prompt); + const generate = page.getByRole('button', { name: /Generate Plan/ }); + await expect(generate).toBeEnabled(); + // Inside the autosave debounce: Generate creates the draft itself. + await generate.click(); + + await expect(page.getByText('Generating', { exact: true })).toBeVisible(); + expect(server.reads).toBeGreaterThan(0); + await expect(page).toHaveURL(new RegExp(`/studio/${draftId}$`)); + await expect(composer).toHaveCount(0); + expect(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth)).toBe(true); + await capture(page, `fresh-plan-generating-${layout.name}`); + + server.complete(); + // With the socket unavailable, the generating view's HTTP fallback picks up completion. + await expect(page.getByText(task.title).first()).toBeVisible({ timeout: 15_000 }); + await expect(page.getByRole('button', { name: /Generate Plan/ })).toHaveCount(0); + expect(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth)).toBe(true); + await capture(page, `fresh-plan-review-${layout.name}`); + expect(server.creations).toBe(1); + expect(server.generations).toBe(1); + + // /studio/new was replaced: back returns to the plans list and forward reopens the finished plan. + await page.goBack(); + await expect(page).toHaveURL(/\/plans$/); + await page.goForward(); + await expect(page).toHaveURL(new RegExp(`/studio/${draftId}$`)); + await expect(page.getByText(task.title).first()).toBeVisible(); + expect(server.generations).toBe(1); + }); +} diff --git a/propr-ui/src/components/TaskPlanner/SetupWizard.tsx b/propr-ui/src/components/TaskPlanner/SetupWizard.tsx index b2e872155..8ef458243 100644 --- a/propr-ui/src/components/TaskPlanner/SetupWizard.tsx +++ b/propr-ui/src/components/TaskPlanner/SetupWizard.tsx @@ -51,7 +51,7 @@ interface SetupWizardProps { onGenerateComplete: () => void; onDraftCreated?: (draftId: string) => void; onDraftCreatedInPlace?: (draft: PlannerDraft) => void; - onGenerationStarted?: (runId: string) => void; + onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void; } @@ -389,7 +389,7 @@ export const SetupWizard: React.FC = ({ draft, onGenerateCompl // Generate shares autosave's single-flight creation so one click cannot persist a second draft const handleCreateDraftAndGenerate = useDraftCreation({ selectedRepo: repoLoader.selectedRepo, config, localFiles: fileHandling.localFiles, - ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, navigate, setError, setIsCreating + ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating }); const autoResize = useAutoResize(textareaRef); const handleRepoChangeInEditMode = useRepoChangeInEditMode({ draft, config, locationTodoIds: locationState?.todoIds, navigate, onDraftCreated, setError, setIsCreating }); diff --git a/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx b/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx index e067eb087..a27c71f4b 100644 --- a/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx +++ b/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx @@ -176,7 +176,7 @@ describe('useGenerationHandlers', () => { }); await act(async () => firstStart); - expect(onGenerationStarted).toHaveBeenCalledWith('generation-run-2'); + expect(onGenerationStarted).toHaveBeenCalledWith('generation-run-2', expect.objectContaining({ draft_id: makeDraft().draft_id })); expect(startPolling).toHaveBeenCalledWith('generation-run-2'); expect(result.current.isStartingGeneration).toBe(false); }); diff --git a/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts b/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts index 0f77a78ab..5ea8b7ce6 100644 --- a/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts +++ b/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts @@ -28,7 +28,7 @@ export interface PlannerConfig { prompt: string; baseBranch: string; granularity export interface PromptPersistedUpdate { draftId: string; initial_prompt: string; name: string; } interface RepoInfoState { isLoading: boolean; error: string | null; } -interface GenerationHandlersParams { draft: PlannerDraft | undefined; config: PlannerConfig; branchError: string | null; flushPrompt?: (draftId: string, prompt: string) => Promise; contextHelpers: { isContextStale: boolean; clearCountdown: () => void; fetchPreview: () => Promise }; startPolling: (runId?: string) => void; stopPolling: () => void; onGenerationStarted?: (runId: string) => void; setError: React.Dispatch>; setGenerationError: (error: string | null) => void; } +interface GenerationHandlersParams { draft: PlannerDraft | undefined; config: PlannerConfig; branchError: string | null; flushPrompt?: (draftId: string, prompt: string) => Promise; contextHelpers: { isContextStale: boolean; clearCountdown: () => void; fetchPreview: () => Promise }; startPolling: (runId?: string) => void; stopPolling: () => void; onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; setError: React.Dispatch>; setGenerationError: (error: string | null) => void; } interface GenerateDisabledParams { isNewMode: boolean; isCreating: boolean; selectedRepo: string; promptTrimmed: string; reposLoading: boolean; isGenerating: boolean; branchError: string | null; repoInfoLoading: boolean; repoError: string | null; baseBranch: string; } type PlannerConfigSetter = React.Dispatch>; @@ -236,7 +236,7 @@ export function useGenerationHandlers({ draft, config, branchError, flushPrompt } } const generation = await generatePlan(draft.draft_id, buildGenerationPayload(config)); - onGenerationStarted?.(generation.runId); + onGenerationStarted?.(generation.runId, draft); startPolling(generation.runId); } catch (err) { stopPolling(); diff --git a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts index 176f142bd..4d65b304f 100644 --- a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts +++ b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts @@ -14,6 +14,9 @@ export interface DraftCreationParams { ensureDraftCreated: () => Promise; uploadPendingLocalFiles: (draftId: string, files: File[]) => Promise; flushPrompt: (draftId: string, prompt: string) => Promise; + // Hands the acknowledged run to the page's draft before navigating; the studio instance survives + // /studio/new → /studio/:id, so router state alone cannot replace the draft it already holds. + onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; navigate: (path: string, options?: { replace?: boolean; state?: unknown }) => void; setError: React.Dispatch>; setIsCreating: React.Dispatch>; @@ -35,7 +38,7 @@ async function syncDraftSetup(draft: PlannerDraft, config: PlannerConfig): Promi } // Generate in new mode: persist the draft through the same creation autosave uses, then start generation for it. -export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, navigate, setError, setIsCreating }: DraftCreationParams) { +export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating }: DraftCreationParams) { const isStartingRef = useRef(false); return useCallback(async () => { if (!selectedRepo || !config.prompt.trim()) return void setError('Please select a repository and enter a prompt'); @@ -55,6 +58,7 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft const draftWithPlan = constructDraftWithPlan({ ...draft, initial_prompt: prompt }, getDraftSetupSnapshot(config)); draftWithPlan.status = 'generating'; draftWithPlan.generation_trace = { steps: [], runId: generation.runId }; + onGenerationStarted?.(generation.runId, draftWithPlan); navigate(`/studio/${draft.draft_id}`, { replace: true, state: { initialDraft: draftWithPlan, initialBaseBranch: config.baseBranch, baseBranchPersistenceWarning } }); } catch (err) { setError((err as Error).message || 'Failed to create draft'); @@ -62,5 +66,5 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft } finally { isStartingRef.current = false; } - }, [selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, navigate, setError, setIsCreating]); + }, [selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating]); } diff --git a/propr-ui/src/hooks/useDraft.test.tsx b/propr-ui/src/hooks/useDraft.test.tsx index daf6d92c4..0fc5e2f51 100644 --- a/propr-ui/src/hooks/useDraft.test.tsx +++ b/propr-ui/src/hooks/useDraft.test.tsx @@ -245,6 +245,59 @@ describe('useDraft', () => { ]); }); + it('activates a fresh run from its acknowledged snapshot while the first fetch is in flight', async () => { + const staleFetch = createDeferred>>(); + mockGetDraft.mockImplementationOnce(() => staleFetch.promise); + const snapshot = { + draft_id: 'draft-1', repository: 'integry/propr', initial_prompt: 'Fresh prompt', status: 'draft', attachments: [], created_at: '2026-10-22T22:00:00Z', + }; + const { result } = renderHook(() => useDraft('draft-1')); + expect(result.current.loading).toBe(true); + + act(() => result.current.activateGenerationRun('generation-run-1', snapshot as never)); + expect(result.current.loading).toBe(false); + expect(result.current.draft?.status).toBe('generating'); + expect(result.current.draft?.initial_prompt).toBe('Fresh prompt'); + expect(result.current.draft?.generation_trace?.runId).toBe('generation-run-1'); + + // The fetch requested before generation started returns the pre-generation draft. + await act(async () => staleFetch.resolve({ ...snapshot, status: 'draft' } as never)); + expect(result.current.draft?.status).toBe('generating'); + expect(result.current.draft?.generation_trace?.runId).toBe('generation-run-1'); + + // A resync requested after the acknowledgement is authoritative. + mockGetDraft.mockResolvedValueOnce({ ...snapshot, status: 'review', generation_trace: { runId: 'generation-run-1', steps: [] } } as never); + await act(async () => result.current.refetch()); + expect(result.current.draft?.status).toBe('review'); + }); + + it('resyncs a generating router snapshot once when the entry is revisited', async () => { + mockGetDraft.mockResolvedValueOnce({ + draft_id: 'draft-1', repository: 'integry/propr', initial_prompt: 'Test prompt', status: 'review', attachments: [], + created_at: '2026-05-05T00:00:00Z', generation_trace: { runId: 'generation-run-1', steps: [] }, + }); + const initialDraft = { + draft_id: 'draft-1', repository: 'integry/propr', initial_prompt: 'Test prompt', status: 'generating', attachments: [], + created_at: '2026-05-05T00:00:00Z', generation_trace: { runId: 'generation-run-1', steps: [] }, + }; + const { result } = renderHook(() => useDraft('draft-1', { initialData: initialDraft as never })); + expect(result.current.loading).toBe(false); + expect(result.current.draft?.status).toBe('generating'); + + await waitFor(() => expect(result.current.draft?.status).toBe('review')); + expect(result.current.loading).toBe(false); + expect(mockGetDraft).toHaveBeenCalledTimes(1); + }); + + it('does not replace the current draft with a snapshot of another draft', async () => { + const { result } = renderHook(() => useDraft('draft-1')); + await waitFor(() => expect(result.current.draft?.draft_id).toBe('draft-1')); + + act(() => result.current.activateGenerationRun('generation-run-9', { draft_id: 'draft-2', status: 'draft' } as never)); + expect(result.current.draft?.draft_id).toBe('draft-1'); + expect(result.current.draft?.generation_trace?.runId).toBe('generation-run-9'); + }); + it('applies terminal draft status from the socket before the follow-up fetch resolves', async () => { const deferredFetch = createDeferred>>(); diff --git a/propr-ui/src/hooks/useDraft.ts b/propr-ui/src/hooks/useDraft.ts index d06d6e24e..d154595c3 100644 --- a/propr-ui/src/hooks/useDraft.ts +++ b/propr-ui/src/hooks/useDraft.ts @@ -12,7 +12,7 @@ interface UseDraftResult { loading: boolean; error: string | null; refetch: () => Promise; - activateGenerationRun: (runId: string) => void; + activateGenerationRun: (runId: string, snapshot?: PlannerDraft) => void; } function ensureArray(value: unknown): unknown[] { @@ -94,22 +94,31 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra const [error, setError] = useState(null); const activeRunIdRef = useRef(draft?.generation_trace?.runId); activeRunIdRef.current = draft?.generation_trace?.runId; + // Callers may hold an activation callback from a render before an in-place draft id arrived. + const draftIdRef = useRef(draftId); + draftIdRef.current = draftId; + // Bumped when a run is acknowledged locally; a fetch requested before that can only return + // the pre-generation snapshot and must not replace the acknowledged run. + const activationSeqRef = useRef(0); // Fetch function - only sets loading for explicit refetch, not background refresh const fetchDraft = useCallback(async (showLoading = true) => { if (!draftId) return; + const activationSeq = activationSeqRef.current; + const isSuperseded = () => activationSeq !== activationSeqRef.current; try { if (showLoading) { setLoading(true); } setError(null); const data = await getDraft(draftId); + if (isSuperseded()) return; const parsedDraft = parseJsonFields(data as unknown as Record) as unknown as PlannerDraft; activeRunIdRef.current = parsedDraft.generation_trace?.runId; setDraft(parsedDraft); } catch (err) { - setError((err as Error).message || 'Failed to fetch draft'); + if (!isSuperseded()) setError((err as Error).message || 'Failed to fetch draft'); } finally { if (showLoading) { setLoading(false); @@ -119,9 +128,12 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra // Initial fetch effect - handles both initial load and draftId changes useEffect(() => { - // If we already have draft data (from initialData), skip the fetch + // If we already have draft data (from initialData), skip the loading fetch // The draft state was set during initialization if (draft && draft.draft_id === draftId) { + // A generating snapshot from router state is stale when the entry is revisited via + // back/forward or reload; resync once so a finished run is not shown as in progress. + if (draft.status === 'generating') void fetchDraft(false); return; } // When draftId changes to a different value, reset state and fetch new data @@ -155,17 +167,29 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra }); }, []); - const activateGenerationRun = useCallback((runId: string) => { + // The snapshot stands in for the draft while its first fetch is still in flight. + const activateGenerationRun = useCallback((runId: string, snapshot?: PlannerDraft) => { + const currentDraftId = draftIdRef.current; + if (!currentDraftId) return; + const hasSnapshot = snapshot?.draft_id === currentDraftId; activeRunIdRef.current = runId; + if (hasSnapshot) activationSeqRef.current += 1; setDraft(currentDraft => { - if (!currentDraft || currentDraft.draft_id !== draftId) return currentDraft; + const base = currentDraft?.draft_id === currentDraftId + ? currentDraft + : (hasSnapshot ? snapshot : null); + if (!base) return currentDraft; return { - ...currentDraft, + ...base, status: 'generating', generation_trace: { steps: [], runId }, }; }); - }, [draftId]); + if (hasSnapshot) { + setError(null); + setLoading(false); + } + }, []); // Handle draft update from WebSocket const handleDraftUpdate = useCallback(async (payload: DraftUpdatePayload) => { diff --git a/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx b/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx new file mode 100644 index 000000000..d9d3c597c --- /dev/null +++ b/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx @@ -0,0 +1,205 @@ +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { BrowserRouter, HashRouter, Route, Routes } from 'react-router-dom'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { DraftUpdatePayload } from '@propr/shared'; +import PlanStudioPage from './PlanStudioPage'; +import { ToastContext } from '../components/ui/useToast'; +import { createDraft, generatePlan, getDraft, getInstanceCatalog, getRepoBranches, updateDraft, type PlannerDraft } from '../api/proprApi'; + +// Real page, routing, useDraft and SetupWizard; only the network and socket boundaries are replaced. +vi.mock('../api/proprApi', async (importOriginal) => ({ + ...(await importOriginal()), + getInstanceCatalog: vi.fn(), + getRepoBranches: vi.fn(), + createDraft: vi.fn(), + updateDraft: vi.fn(), + uploadAttachment: vi.fn(), + generatePlan: vi.fn(), + getDraft: vi.fn(), + previewContext: vi.fn(async () => ({ + success: true, stats: { totalTokens: 1, costEstimate: 0, contextLength: 1, fileCount: 0 }, smartSelection: [], warnings: [], + })), +})); +vi.mock('../api/repoIndexingApi', async (importOriginal) => ({ + ...(await importOriginal()), + getRepositoriesIndexingStatus: vi.fn(async () => ({ repositories: [] })), +})); +vi.mock('../api/userRepoPreferencesApi', async (importOriginal) => ({ + ...(await importOriginal()), + getUserRepoPreferences: vi.fn(async () => ({})), +})); +vi.mock('../components/TaskPlanner/PlanEditor', () => ({ + default: ({ draft }: { draft: PlannerDraft }) => {`${draft.draft_id}:${draft.status}`}, +})); +const socket = vi.hoisted(() => { + const listeners = new Set<(payload: DraftUpdatePayload) => void>(); + return { + listeners, + value: { + socket: null, isConnected: true, + subscribeToTask: () => {}, unsubscribeFromTask: () => {}, subscribeToDraft: () => {}, unsubscribeFromDraft: () => {}, + subscribeToIndexing: () => {}, unsubscribeFromIndexing: () => {}, subscribeToIndexingUpdates: () => {}, unsubscribeFromIndexingUpdates: () => {}, + onDraftUpdate: (listener: (payload: DraftUpdatePayload) => void) => { listeners.add(listener); return () => listeners.delete(listener); }, + }, + }; +}); +vi.mock('../contexts/useSocket', () => ({ useSocket: () => socket.value, useOptionalSocket: () => socket.value })); + +const PROMPT = 'Add a health endpoint'; +const toast = { addToast: vi.fn(() => 'toast'), removeToast: vi.fn() }; +const renderer = 'propr-app://renderer/renderer.html'; + +const deferred = () => { + let resolve!: (value: T) => void; + const promise = new Promise(res => { resolve = res; }); + return { promise, resolve }; +}; +const serverDraft = (status: string, runId?: string): PlannerDraft => ({ + draft_id: 'draft-1', repository: 'integry/propr', initial_prompt: PROMPT, status, attachments: [], created_at: '2026-10-22T22:00:00Z', + ...(runId ? { generation_trace: { runId, steps: [] } } : {}), + ...(status === 'review' ? { plan_json: [{ id: 'task-1', title: 'Health endpoint', body: 'Body', implementation: 'Impl' }] } : {}), +} as PlannerDraft); +const reconfigureUrl = (url: string) => { + (globalThis as unknown as { jsdom: { reconfigure(options: { url: string }): void } }).jsdom.reconfigure({ url }); +}; +const emit = (payload: Partial) => act(() => { + for (const listener of socket.listeners) listener({ draftId: 'draft-1', timestamp: new Date().toISOString(), ...payload } as DraftUpdatePayload); +}); + +// Mirrors App.tsx: both studio routes render the same element tree, so the page instance survives /studio/new → /studio/:id. +function renderStudio(desktop: boolean) { + vi.stubGlobal('__PROPR_DESKTOP__', desktop); + // Open the studio from the plans list so back/forward has a previous entry to return to. + reconfigureUrl(desktop ? `${renderer}#/plans` : 'https://app.example/plans'); + window.history.pushState(null, '', desktop ? '#/studio/new' : '/studio/new'); + const Router = desktop ? HashRouter : BrowserRouter; + return render( + + + + } /> + } /> + Plans} /> + + + + ); +} + +const generateButton = () => screen.getByRole('button', { name: /generate/i }); +const waitPastAutosaveDebounce = () => act(() => new Promise(resolve => setTimeout(resolve, 1300))); +const historyStep = (step: () => void) => act(async () => { + const popped = new Promise(resolve => window.addEventListener('popstate', resolve, { once: true })); + step(); + await popped; +}); +const studioUrl = (desktop: boolean) => (desktop ? `${renderer}#/studio/draft-1` : 'https://app.example/studio/draft-1'); + +async function enterPrompt() { + const textarea = await screen.findByPlaceholderText(/Describe the feature/); + await waitFor(() => expect(screen.getByTestId('branch-chip')).toHaveAttribute('data-status', 'resolved')); + textarea.focus(); + fireEvent.change(textarea, { target: { value: PROMPT } }); + await waitFor(() => expect(generateButton()).toBeEnabled()); + return textarea; +} + +describe.each([false, true])('fresh plan generation becomes the displayed draft (desktop=%s)', desktop => { + beforeEach(() => { + vi.clearAllMocks(); + socket.listeners.clear(); + localStorage.clear(); + vi.mocked(getInstanceCatalog).mockResolvedValue({ + repositories: [{ name: 'integry/propr', enabled: true, baseBranch: 'main' }], agents: [], + } as never); + vi.mocked(getRepoBranches).mockResolvedValue({ defaultBranch: 'main', branches: ['main'] } as never); + vi.mocked(updateDraft).mockResolvedValue(undefined as never); + vi.mocked(createDraft).mockImplementation(async (_repo, prompt) => ({ ...serverDraft('draft'), initial_prompt: prompt })); + }); + + afterEach(() => { + reconfigureUrl('http://localhost:3000/'); + vi.unstubAllGlobals(); + }); + + // The server claims the run only after preparing the repository, so the page's own read of the new draft answers + // with the pre-generation snapshot, usually before the acknowledgement and sometimes after it. + it.each(['before', 'after'] as const)('shows Generating then Review when Generate races the autosave debounce (stale read %s the acknowledgement)', async staleOrder => { + const staleFetch = deferred(); + vi.mocked(getDraft).mockImplementationOnce(() => staleFetch.promise).mockResolvedValue(serverDraft('review', 'run-1')); + const creation = deferred(); + vi.mocked(createDraft).mockImplementationOnce(() => creation.promise); + const generation = deferred>>(); + vi.mocked(generatePlan).mockImplementationOnce(() => generation.promise); + renderStudio(desktop); + const historyLength = window.history.length; + await enterPrompt(); + + fireEvent.click(generateButton()); + await act(async () => creation.resolve(serverDraft('draft'))); + await waitFor(() => expect(generatePlan).toHaveBeenCalledTimes(1)); + expect(window.location.href).toBe(studioUrl(desktop)); + if (staleOrder === 'before') await act(async () => staleFetch.resolve(serverDraft('draft'))); + await act(async () => generation.resolve({ success: true, status: 'generating', message: 'started', runId: 'run-1' })); + if (staleOrder === 'after') await act(async () => staleFetch.resolve(serverDraft('draft'))); + + expect(await screen.findByText('Generating')).toBeInTheDocument(); + expect(screen.queryByPlaceholderText(/Describe the feature/)).not.toBeInTheDocument(); + expect(window.location.href).toBe(studioUrl(desktop)); + + // A different run cannot complete this one. + await emit({ runId: 'run-0', draftStatus: 'review' }); + expect(screen.queryByTestId('review')).not.toBeInTheDocument(); + await emit({ runId: 'run-1', draftStatus: 'review' }); + expect(await screen.findByTestId('review')).toHaveTextContent('draft-1:review'); + await waitPastAutosaveDebounce(); + expect(createDraft).toHaveBeenCalledTimes(1); + expect(generatePlan).toHaveBeenCalledTimes(1); + expect(window.location.href).toBe(studioUrl(desktop)); + + // /studio/new was replaced, not stacked: back leaves the studio and forward reopens the same plan. + expect(window.history.length).toBe(historyLength); + await historyStep(() => window.history.back()); + expect(await screen.findByRole('heading', { name: 'Plans' })).toBeInTheDocument(); + await historyStep(() => window.history.forward()); + expect(await screen.findByTestId('review')).toHaveTextContent('draft-1:review'); + expect(window.location.href).toBe(studioUrl(desktop)); + expect(generatePlan).toHaveBeenCalledTimes(1); + }); + + it('shows Generating then Review when Generate follows an in-place autosave', async () => { + vi.mocked(getDraft).mockResolvedValueOnce(serverDraft('draft')).mockResolvedValue(serverDraft('review', 'run-1')); + const generation = deferred>>(); + vi.mocked(generatePlan).mockImplementationOnce(() => generation.promise); + renderStudio(desktop); + const textarea = await enterPrompt(); + await waitPastAutosaveDebounce(); + await waitFor(() => expect(window.location.href).toBe(studioUrl(desktop))); + expect(screen.getByPlaceholderText(/Describe the feature/)).toBe(textarea); + expect(textarea).toHaveFocus(); + await waitFor(() => expect(generateButton()).toBeEnabled()); + + fireEvent.click(generateButton()); + await waitFor(() => expect(generatePlan).toHaveBeenCalledTimes(1)); + await act(async () => generation.resolve({ success: true, status: 'generating', message: 'started', runId: 'run-1' })); + + expect(await screen.findByText('Generating')).toBeInTheDocument(); + await emit({ runId: 'run-1', draftStatus: 'review' }); + expect(await screen.findByTestId('review')).toHaveTextContent('draft-1:review'); + expect(createDraft).toHaveBeenCalledTimes(1); + expect(generatePlan).toHaveBeenCalledTimes(1); + }); + + it('keeps the prompt for a retry when generation fails to start', async () => { + vi.mocked(getDraft).mockResolvedValue(serverDraft('draft')); + vi.mocked(generatePlan).mockRejectedValueOnce(new Error('Provider unavailable')); + renderStudio(desktop); + await enterPrompt(); + + fireEvent.click(generateButton()); + expect(await screen.findByText('Provider unavailable')).toBeInTheDocument(); + expect(screen.getByPlaceholderText(/Describe the feature/)).toHaveValue(PROMPT); + expect(screen.queryByText('Generating')).not.toBeInTheDocument(); + expect(createDraft).toHaveBeenCalledTimes(1); + }); +}); diff --git a/propr-ui/src/pages/PlanStudioPage.tsx b/propr-ui/src/pages/PlanStudioPage.tsx index 758e03ec9..66287fd93 100644 --- a/propr-ui/src/pages/PlanStudioPage.tsx +++ b/propr-ui/src/pages/PlanStudioPage.tsx @@ -197,7 +197,7 @@ const ReviewView: React.FC<{ ); -const DraftView: React.FC<{ currentStage: StudioStage; draft: PlannerDraft; onRefetch: () => void; onGenerationStarted: (runId: string) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void }> = ({ currentStage, draft, onRefetch, onGenerationStarted, onDraftMetadataPersisted }) => ( +const DraftView: React.FC<{ currentStage: StudioStage; draft: PlannerDraft; onRefetch: () => void; onGenerationStarted: (runId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void }> = ({ currentStage, draft, onRefetch, onGenerationStarted, onDraftMetadataPersisted }) => ( void; onRefetch?: () => void; - onGenerationStarted?: (runId: string) => void; + onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void; }> = ({ draft, onDraftCreated, onRefetch, onGenerationStarted, onDraftMetadataPersisted, singleTask }) => ( void; - onGenerationStarted: (runId: string) => void; + onGenerationStarted: (runId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted: (update: PromptPersistedUpdate) => void; } From c5c51b32602b24081262f73b8c8c6a7c248555a6 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:28:28 +0000 Subject: [PATCH 2/5] feat(ai): I fixed F1 (a late acknowledgement from one draft could take over another draft) and made the browser test's ordering explicit (S1). The full UI unit suite passes (3058/3058), `tsc` and `eslint --max-warnings 0` are clean, and the fresh-generate Playwright test passes in all 4 layouts. I haven't tried it in the installed native app. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed F1 (a late acknowledgement from one draft could take over another draft) and made the browser test's ordering explicit (S1). The full UI unit suite passes (3058/3058), `tsc` and `eslint --max-warnings 0` are clean, and the fresh-generate Playwright test passes in all 4 layouts. I haven't tried it in the installed native app. **F1 fix** - **`useDraft.ts`:** `activateGenerationRun` now requires the ID of the draft the run was started for: `(runId, draftId, snapshot?)`. If that ID isn't the draft currently shown, or a snapshot belongs to another draft, it returns before touching any run ID, draft, fetch counter, error or loading state. This applies whether or not a snapshot is passed. - **`setupWizardHooks.ts`:** the same mistaken assumption was in the existing-draft Generate handler. The wizard stays mounted when you go from `/studio/A` to `/studio/B`, so A's late acknowledgement used to call `startPolling(runA)` while B was on screen. B's own updates would then be rejected for having the wrong run ID. The handler now checks which draft is shown before handing off the run or polling. - **Callers:** `useDraftCreation.ts`, `SetupWizard.tsx` and `PlanStudioPage.tsx` pass the new draft-ID argument. **Tests** - The old mismatched-snapshot test, which expected A's run ID to be applied to the other draft, is replaced. The new one requires status and run ID to stay unchanged, both with a foreign snapshot and with no snapshot. - New hook test for A → B while A's acknowledgement is pending. A's late acknowledgement arrives twice, once while B's fetch is still running and once after it finishes. B keeps its own status and run ID both times, and still accepts its own live updates. - New wizard-handler test: switching to B before A's acknowledgement means neither the activation callback nor polling runs. - With the fixes temporarily removed, all three new tests fail. **S1, `e2e/planner-fresh-generate.pw.ts`** - The five-second fallback timer is gone. The page's reads now return a name the create response doesn't have, and the generate route holds the acknowledgement until the page's title shows that name. That proves React has rendered the pre-generation read first. - The test asserts that this happened, so a missing read now fails with a clear message instead of passing by timing. - Against the pre-PR source, the test fails at "Generating visible" in all layouts. No preview images: nothing visible changed. **Possible separate issue (not changed, not tested):** in `useDraftCreation.ts`, the fresh-plan flow still does `navigate('/studio/', { replace: true })` after the acknowledgement arrives. If the user leaves `/studio/new` while Generate is pending, it may send them back to the new draft. That code came before this PR and is outside F1. PR: #3325 Comment by: @integry (ID: 6353387716) Model: Claude Opus 5.5 --- propr-ui/e2e/planner-fresh-generate.pw.ts | 19 ++++--- .../components/TaskPlanner/SetupWizard.tsx | 2 +- .../TaskPlanner/setupWizardHooks.test.tsx | 32 ++++++++++- .../TaskPlanner/setupWizardHooks.ts | 8 ++- .../TaskPlanner/useDraftCreation.ts | 4 +- propr-ui/src/hooks/useDraft.test.tsx | 53 +++++++++++++++++-- propr-ui/src/hooks/useDraft.ts | 19 ++++--- propr-ui/src/pages/PlanStudioPage.tsx | 6 +-- 8 files changed, 112 insertions(+), 31 deletions(-) diff --git a/propr-ui/e2e/planner-fresh-generate.pw.ts b/propr-ui/e2e/planner-fresh-generate.pw.ts index 02884d4cd..ac543b3e4 100644 --- a/propr-ui/e2e/planner-fresh-generate.pw.ts +++ b/propr-ui/e2e/planner-fresh-generate.pw.ts @@ -4,6 +4,7 @@ import { installWindowActions } from './native-chrome.fixture'; const draftId = 'fresh-plan'; const prompt = 'Add a health endpoint to the API'; +const serverName = 'Fresh draft named by the server'; const task = { id: 'task-1', title: 'Add a GET /health endpoint with a readiness check', body: '## Context\nOne bounded task.', implementation: '' }; const layouts = [ @@ -36,15 +37,16 @@ async function installNativeShell(page: Page) { * A server for one fresh draft. Like the real one, POST /generate claims the run only after preparing the repository, * which takes longer than the page's own read of the new draft: that read answers with the pre-generation draft * before the acknowledgement arrives. Completion is up to the test. + * + * Reads carry a server-assigned name the create response lacks, so the document title shows when the page has applied + * a read; the acknowledgement is held until then rather than for a fixed time. */ async function routeFreshDraft(page: Page) { - const server = { creations: 0, generations: 0, reads: 0, complete: () => {} }; + const server = { creations: 0, generations: 0, reads: 0, readAppliedBeforeAck: false, complete: () => {} }; const draft: Record = { draft_id: draftId, repository: 'integry/propr', initial_prompt: '', status: 'draft', attachments: [], chat_history: [], plan_json: [], created_at: '2026-10-22T22:00:00Z', context_config: { baseBranch: 'main' }, }; - let answerFirstRead!: () => void; - const firstReadAnswered = new Promise(resolve => { answerFirstRead = resolve; }); server.complete = () => Object.assign(draft, { status: 'review', plan_json: [task], name: 'Health endpoint', generation_trace: { runId: 'run-1', steps: [] } }); await page.route('**/api/planner/drafts', route => { if (route.request().method() !== 'POST') return route.fallback(); @@ -57,8 +59,8 @@ async function routeFreshDraft(page: Page) { Object.assign(draft, route.request().postDataJSON()); return route.fulfill({ json: draft }); } - await route.fulfill({ json: draft }); - if (++server.reads === 1) answerFirstRead(); + server.reads++; + await route.fulfill({ json: { ...draft, name: draft.name ?? serverName } }); }); await page.route(`**/api/planner/drafts/${draftId}/repository-info`, route => route.fulfill({ json: { defaultBranch: 'main', branches: ['main'] } })); await page.route('**/api/github/repos/integry/propr/branches', route => route.fulfill({ json: { defaultBranch: 'main', branches: ['main'] } })); @@ -68,7 +70,9 @@ async function routeFreshDraft(page: Page) { await page.route('**/api/planner/generate', async route => { server.generations++; expect(route.request().postDataJSON().draftId).toBe(draftId); - await Promise.race([firstReadAnswered, new Promise(resolve => setTimeout(resolve, 5_000))]); + // Barrier: the page has rendered the pre-generation read. On timeout the acknowledgement still goes out and the test's assertion fails. + server.readAppliedBeforeAck = await page.waitForFunction(name => document.title.includes(name), serverName, { timeout: 10_000 }) + .then(() => true, () => false); Object.assign(draft, { status: 'generating', generation_trace: { runId: 'run-1', steps: [{ name: 'relevance', status: 'in_progress' }] } }); await route.fulfill({ json: { success: true, runId: 'run-1', status: 'generating', message: 'started' } }); }); @@ -93,7 +97,8 @@ for (const layout of layouts) { await generate.click(); await expect(page.getByText('Generating', { exact: true })).toBeVisible(); - expect(server.reads).toBeGreaterThan(0); + expect(server.generations).toBe(1); + expect(server.readAppliedBeforeAck).toBe(true); await expect(page).toHaveURL(new RegExp(`/studio/${draftId}$`)); await expect(composer).toHaveCount(0); expect(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth)).toBe(true); diff --git a/propr-ui/src/components/TaskPlanner/SetupWizard.tsx b/propr-ui/src/components/TaskPlanner/SetupWizard.tsx index 8ef458243..692a526c4 100644 --- a/propr-ui/src/components/TaskPlanner/SetupWizard.tsx +++ b/propr-ui/src/components/TaskPlanner/SetupWizard.tsx @@ -51,7 +51,7 @@ interface SetupWizardProps { onGenerateComplete: () => void; onDraftCreated?: (draftId: string) => void; onDraftCreatedInPlace?: (draft: PlannerDraft) => void; - onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; + onGenerationStarted?: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void; } diff --git a/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx b/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx index a27c71f4b..107ceec3b 100644 --- a/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx +++ b/propr-ui/src/components/TaskPlanner/setupWizardHooks.test.tsx @@ -176,11 +176,41 @@ describe('useGenerationHandlers', () => { }); await act(async () => firstStart); - expect(onGenerationStarted).toHaveBeenCalledWith('generation-run-2', expect.objectContaining({ draft_id: makeDraft().draft_id })); + expect(onGenerationStarted).toHaveBeenCalledWith('generation-run-2', makeDraft().draft_id, expect.objectContaining({ draft_id: makeDraft().draft_id })); expect(startPolling).toHaveBeenCalledWith('generation-run-2'); expect(result.current.isStartingGeneration).toBe(false); }); + it('drops an acknowledgement that arrives after the wizard switched to another draft', async () => { + const generationResponse = createDeferred>>(); + mockGeneratePlan.mockImplementationOnce(() => generationResponse.promise); + const startPolling = vi.fn(); + const onGenerationStarted = vi.fn(); + const { result, rerender } = renderHook(({ draft }: { draft: PlannerDraft }) => useGenerationHandlers({ + draft, + config: baseConfig, + branchError: null, + contextHelpers: { isContextStale: false, clearCountdown: vi.fn(), fetchPreview: vi.fn() }, + startPolling, + stopPolling: vi.fn(), + onGenerationStarted, + setError: vi.fn(), + setGenerationError: vi.fn(), + }), { initialProps: { draft: makeDraft() as never } }); + + let start!: Promise; + act(() => { start = result.current.handleGenerateForExistingDraft(); }); + await waitFor(() => expect(mockGeneratePlan).toHaveBeenCalledWith('draft-1', expect.anything())); + + rerender({ draft: { ...makeDraft(), draft_id: 'draft-2' } as never }); + generationResponse.resolve({ success: true, status: 'generating', message: 'Plan generation started', runId: 'generation-run-a' }); + await act(async () => start); + + expect(onGenerationStarted).not.toHaveBeenCalled(); + expect(startPolling).not.toHaveBeenCalled(); + expect(result.current.isStartingGeneration).toBe(false); + }); + it('does not generate when the required context preview fails', async () => { const startPolling = vi.fn(); const setError = vi.fn(); diff --git a/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts b/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts index 5ea8b7ce6..3cc89a1a7 100644 --- a/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts +++ b/propr-ui/src/components/TaskPlanner/setupWizardHooks.ts @@ -28,7 +28,7 @@ export interface PlannerConfig { prompt: string; baseBranch: string; granularity export interface PromptPersistedUpdate { draftId: string; initial_prompt: string; name: string; } interface RepoInfoState { isLoading: boolean; error: string | null; } -interface GenerationHandlersParams { draft: PlannerDraft | undefined; config: PlannerConfig; branchError: string | null; flushPrompt?: (draftId: string, prompt: string) => Promise; contextHelpers: { isContextStale: boolean; clearCountdown: () => void; fetchPreview: () => Promise }; startPolling: (runId?: string) => void; stopPolling: () => void; onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; setError: React.Dispatch>; setGenerationError: (error: string | null) => void; } +interface GenerationHandlersParams { draft: PlannerDraft | undefined; config: PlannerConfig; branchError: string | null; flushPrompt?: (draftId: string, prompt: string) => Promise; contextHelpers: { isContextStale: boolean; clearCountdown: () => void; fetchPreview: () => Promise }; startPolling: (runId?: string) => void; stopPolling: () => void; onGenerationStarted?: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; setError: React.Dispatch>; setGenerationError: (error: string | null) => void; } interface GenerateDisabledParams { isNewMode: boolean; isCreating: boolean; selectedRepo: string; promptTrimmed: string; reposLoading: boolean; isGenerating: boolean; branchError: string | null; repoInfoLoading: boolean; repoError: string | null; baseBranch: string; } type PlannerConfigSetter = React.Dispatch>; @@ -217,6 +217,9 @@ export function useGenerationHandlers({ draft, config, branchError, flushPrompt const isStartingGenerationRef = useRef(false); const contextHelpersRef = useRef(contextHelpers); contextHelpersRef.current = contextHelpers; + // The wizard survives /studio/:a → /studio/:b, so an acknowledgement can arrive for a draft no longer shown. + const currentDraftIdRef = useRef(draft?.draft_id); + currentDraftIdRef.current = draft?.draft_id; const handleGenerateForExistingDraft = useCallback(async () => { if (!draft || isStartingGenerationRef.current) return; if (branchError) return void setError('Please fix the branch name before generating'); @@ -236,7 +239,8 @@ export function useGenerationHandlers({ draft, config, branchError, flushPrompt } } const generation = await generatePlan(draft.draft_id, buildGenerationPayload(config)); - onGenerationStarted?.(generation.runId, draft); + if (currentDraftIdRef.current !== draft.draft_id) return; + onGenerationStarted?.(generation.runId, draft.draft_id, draft); startPolling(generation.runId); } catch (err) { stopPolling(); diff --git a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts index 4d65b304f..2ba7e72af 100644 --- a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts +++ b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts @@ -16,7 +16,7 @@ export interface DraftCreationParams { flushPrompt: (draftId: string, prompt: string) => Promise; // Hands the acknowledged run to the page's draft before navigating; the studio instance survives // /studio/new → /studio/:id, so router state alone cannot replace the draft it already holds. - onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; + onGenerationStarted?: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; navigate: (path: string, options?: { replace?: boolean; state?: unknown }) => void; setError: React.Dispatch>; setIsCreating: React.Dispatch>; @@ -58,7 +58,7 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft const draftWithPlan = constructDraftWithPlan({ ...draft, initial_prompt: prompt }, getDraftSetupSnapshot(config)); draftWithPlan.status = 'generating'; draftWithPlan.generation_trace = { steps: [], runId: generation.runId }; - onGenerationStarted?.(generation.runId, draftWithPlan); + onGenerationStarted?.(generation.runId, draft.draft_id, draftWithPlan); navigate(`/studio/${draft.draft_id}`, { replace: true, state: { initialDraft: draftWithPlan, initialBaseBranch: config.baseBranch, baseBranchPersistenceWarning } }); } catch (err) { setError((err as Error).message || 'Failed to create draft'); diff --git a/propr-ui/src/hooks/useDraft.test.tsx b/propr-ui/src/hooks/useDraft.test.tsx index 0fc5e2f51..9284d2004 100644 --- a/propr-ui/src/hooks/useDraft.test.tsx +++ b/propr-ui/src/hooks/useDraft.test.tsx @@ -220,7 +220,7 @@ describe('useDraft', () => { }; const { result } = renderHook(() => useDraft('draft-1', { initialData: initialDraft as never })); - act(() => result.current.activateGenerationRun('generation-run-2')); + act(() => result.current.activateGenerationRun('generation-run-2', 'draft-1')); await waitFor(() => expect(draftUpdateListeners.size).toBeGreaterThan(0)); await act(async () => { await Promise.all([...draftUpdateListeners].map(listener => listener({ @@ -254,7 +254,7 @@ describe('useDraft', () => { const { result } = renderHook(() => useDraft('draft-1')); expect(result.current.loading).toBe(true); - act(() => result.current.activateGenerationRun('generation-run-1', snapshot as never)); + act(() => result.current.activateGenerationRun('generation-run-1', 'draft-1', snapshot as never)); expect(result.current.loading).toBe(false); expect(result.current.draft?.status).toBe('generating'); expect(result.current.draft?.initial_prompt).toBe('Fresh prompt'); @@ -289,13 +289,56 @@ describe('useDraft', () => { expect(mockGetDraft).toHaveBeenCalledTimes(1); }); - it('does not replace the current draft with a snapshot of another draft', async () => { + it('ignores an acknowledgement whose snapshot belongs to another draft', async () => { + mockGetDraft.mockResolvedValueOnce({ + draft_id: 'draft-1', repository: 'integry/propr', initial_prompt: 'Test prompt', status: 'draft', attachments: [], created_at: '2026-05-05T00:00:00Z', + }); const { result } = renderHook(() => useDraft('draft-1')); await waitFor(() => expect(result.current.draft?.draft_id).toBe('draft-1')); - act(() => result.current.activateGenerationRun('generation-run-9', { draft_id: 'draft-2', status: 'draft' } as never)); + act(() => result.current.activateGenerationRun('generation-run-9', 'draft-1', { draft_id: 'draft-2', status: 'draft' } as never)); + act(() => result.current.activateGenerationRun('generation-run-9', 'draft-2')); expect(result.current.draft?.draft_id).toBe('draft-1'); - expect(result.current.draft?.generation_trace?.runId).toBe('generation-run-9'); + expect(result.current.draft?.status).toBe('draft'); + expect(result.current.draft?.generation_trace?.runId).toBeUndefined(); + }); + + it('ignores the acknowledgement for draft A after switching to draft B', async () => { + const draftA = { + draft_id: 'draft-a', repository: 'integry/propr', initial_prompt: 'Prompt A', status: 'draft', attachments: [], created_at: '2026-10-22T22:00:00Z', + }; + const draftB = { + draft_id: 'draft-b', repository: 'integry/propr', initial_prompt: 'Prompt B', status: 'generating', attachments: [], + created_at: '2026-10-22T22:00:00Z', generation_trace: { runId: 'generation-run-b', steps: [] }, + }; + mockGetDraft.mockResolvedValueOnce(draftA as never); + const { result, rerender } = renderHook(({ id }) => useDraft(id), { initialProps: { id: 'draft-a' } }); + await waitFor(() => expect(result.current.draft?.draft_id).toBe('draft-a')); + // The activation callback captured while A's generate request was pending. + const activateFromA = result.current.activateGenerationRun; + + const draftBFetch = createDeferred>>(); + mockGetDraft.mockImplementationOnce(() => draftBFetch.promise); + rerender({ id: 'draft-b' }); + + // A's acknowledgement lands while B's fetch is still in flight, then again after it settles. + act(() => activateFromA('generation-run-a', 'draft-a', draftA as never)); + await act(async () => draftBFetch.resolve(draftB as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', status: 'generating', generation_trace: { runId: 'generation-run-b' } }); + + act(() => activateFromA('generation-run-a', 'draft-a', draftA as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', status: 'generating', generation_trace: { runId: 'generation-run-b' } }); + + // B's own socket updates are still accepted under B's run ID. + await act(async () => { + for (const listener of draftUpdateListeners) { + await listener({ + draftId: 'draft-b', runId: 'generation-run-b', draftStatus: 'generating', timestamp: '2026-10-22T22:01:00Z', + generationTrace: { runId: 'generation-run-b', steps: [{ name: 'context', status: 'in_progress' }] }, + } as never); + } + }); + expect(result.current.draft?.generation_trace?.steps).toEqual([{ name: 'context', status: 'in_progress' }]); }); it('applies terminal draft status from the socket before the follow-up fetch resolves', async () => { diff --git a/propr-ui/src/hooks/useDraft.ts b/propr-ui/src/hooks/useDraft.ts index d154595c3..17397e154 100644 --- a/propr-ui/src/hooks/useDraft.ts +++ b/propr-ui/src/hooks/useDraft.ts @@ -12,7 +12,7 @@ interface UseDraftResult { loading: boolean; error: string | null; refetch: () => Promise; - activateGenerationRun: (runId: string, snapshot?: PlannerDraft) => void; + activateGenerationRun: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; } function ensureArray(value: unknown): unknown[] { @@ -167,17 +167,16 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra }); }, []); - // The snapshot stands in for the draft while its first fetch is still in flight. - const activateGenerationRun = useCallback((runId: string, snapshot?: PlannerDraft) => { + // draftId names the draft the run was started for; a delayed acknowledgement for another draft + // is ignored. The snapshot stands in for the draft while its first fetch is still in flight. + const activateGenerationRun = useCallback((runId: string, runDraftId: string, snapshot?: PlannerDraft) => { const currentDraftId = draftIdRef.current; - if (!currentDraftId) return; - const hasSnapshot = snapshot?.draft_id === currentDraftId; + if (!currentDraftId || runDraftId !== currentDraftId) return; + if (snapshot && snapshot.draft_id !== currentDraftId) return; activeRunIdRef.current = runId; - if (hasSnapshot) activationSeqRef.current += 1; + if (snapshot) activationSeqRef.current += 1; setDraft(currentDraft => { - const base = currentDraft?.draft_id === currentDraftId - ? currentDraft - : (hasSnapshot ? snapshot : null); + const base = currentDraft?.draft_id === currentDraftId ? currentDraft : snapshot; if (!base) return currentDraft; return { ...base, @@ -185,7 +184,7 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra generation_trace: { steps: [], runId }, }; }); - if (hasSnapshot) { + if (snapshot) { setError(null); setLoading(false); } diff --git a/propr-ui/src/pages/PlanStudioPage.tsx b/propr-ui/src/pages/PlanStudioPage.tsx index 66287fd93..962dfa4c8 100644 --- a/propr-ui/src/pages/PlanStudioPage.tsx +++ b/propr-ui/src/pages/PlanStudioPage.tsx @@ -197,7 +197,7 @@ const ReviewView: React.FC<{ ); -const DraftView: React.FC<{ currentStage: StudioStage; draft: PlannerDraft; onRefetch: () => void; onGenerationStarted: (runId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void }> = ({ currentStage, draft, onRefetch, onGenerationStarted, onDraftMetadataPersisted }) => ( +const DraftView: React.FC<{ currentStage: StudioStage; draft: PlannerDraft; onRefetch: () => void; onGenerationStarted: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void }> = ({ currentStage, draft, onRefetch, onGenerationStarted, onDraftMetadataPersisted }) => ( void; onRefetch?: () => void; - onGenerationStarted?: (runId: string, snapshot?: PlannerDraft) => void; + onGenerationStarted?: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void; }> = ({ draft, onDraftCreated, onRefetch, onGenerationStarted, onDraftMetadataPersisted, singleTask }) => ( void; - onGenerationStarted: (runId: string, snapshot?: PlannerDraft) => void; + onGenerationStarted: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted: (update: PromptPersistedUpdate) => void; } From af6936aca92d691a8ab661e78af4db9297e3aef3 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:51:34 +0000 Subject: [PATCH 3/5] feat(ai): I fixed the shard 2/4 failure: the test file now passes locally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. I fixed the shard 2/4 failure: the test file now passes locally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. **Cause.** Only `test/prPublicationRecoveryJob.test.ts` failed in shard 2. It didn't fail on an assertion; the test crashed on load because of a missing import: ``` SyntaxError: The requested module './prCommentCollisionRecovery.js' does not provide an export named 'delayPRCommentJobInPlace' ``` The jobs fix f5e35100f, which this branch includes, made `src/jobs/prCommentHandoffIntegration.ts` import two new functions, `delayPRCommentJobInPlace` and `predecessorWaitDelay`. This test replaces that module with a stand-in that lists every function by name, and those two weren't on the list. That commit isn't part of the planner fix, so this failure comes from the base the branch was cut from, not from the F1/S1 changes. **Fix.** I added the two missing functions to the stand-in at `test/prPublicationRecoveryJob.test.ts:309`, copying the change main already made in #3321 (960a4375d). The branch was cut before that commit, so it didn't have it. No product code changed and nothing visible changed, so there are no previews. PR: #3325 Comment by: @propr-dev[bot] (ID: 6353859686) Model: Claude Opus 5.5 --- test/prPublicationRecoveryJob.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/prPublicationRecoveryJob.test.ts b/test/prPublicationRecoveryJob.test.ts index 62c06ce96..b65caea42 100644 --- a/test/prPublicationRecoveryJob.test.ts +++ b/test/prPublicationRecoveryJob.test.ts @@ -306,7 +306,7 @@ const modules: Record> = { ensurePRProcessingLockToken: async () => 'token', releasePRProcessingLock: async (_redis: unknown, key: string) => { heldLocks.delete(key); }, startPRProcessingLockHeartbeat: () => noOp, }, - prCommentCollisionRecovery: { createPRCommentTaskStateIfMissing: async ({ taskId }: { taskId: string }) => { if (!taskStates.has(taskId)) taskStates.set(taskId, 'processing'); }, evaluatePRCommentPreExecutionRecovery: async () => ({}), handlePRCommentLockContention: async () => ({ status: 'deferred' }), schedulePRCommentRecovery: async () => undefined }, + prCommentCollisionRecovery: { createPRCommentTaskStateIfMissing: async ({ taskId }: { taskId: string }) => { if (!taskStates.has(taskId)) taskStates.set(taskId, 'processing'); }, evaluatePRCommentPreExecutionRecovery: async () => ({}), handlePRCommentLockContention: async () => ({ status: 'deferred' }), schedulePRCommentRecovery: async () => undefined, delayPRCommentJobInPlace: async () => false, predecessorWaitDelay: () => 60_000 }, prCompletionComment: { buildCompletionComment: async (commit: unknown, comments: unknown, options: unknown, result: unknown) => JSON.stringify({ commit, comments, options, result }), noChangeAnalysisSummary: () => '' }, reviewCommentGatherer: { markReviewFindingsProcessed: noOp }, // Follow-up assignment is covered by test/prAutoAssignmentFollowup.test.ts. From 65e4b864fc2594330f8388b80359eddc74ab9105 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:20:59 +0000 Subject: [PATCH 4/5] feat(ai): I fixed S3. If the user leaves a fresh plan while its generation acknowledgement is still pending, the late acknowledgement no longer navigates, replaces their history entry or touches another draft. Generation isn't cancelled: the run stays on the server, and reopening the draft shows Generating and then Review, with one create and one generation. The new tests pass in real Chromium for all four layouts. I haven't tried it in the installed native app. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed S3. If the user leaves a fresh plan while its generation acknowledgement is still pending, the late acknowledgement no longer navigates, replaces their history entry or touches another draft. Generation isn't cancelled: the run stays on the server, and reopening the draft shows Generating and then Review, with one create and one generation. The new tests pass in real Chromium for all four layouts. I haven't tried it in the installed native app. **What was wrong.** After the generate response arrived, the fresh-plan flow always called `navigate('/studio/', { replace: true })`, even if the user had left. My first fix checked the router's location and whether the page was still mounted. It passed the unit tests but failed in the real browser. In the production build, Back loads the `/plans` page in the background before switching, so for a moment the studio is still mounted and the router still reports the old location. The browser's history has already moved by then. **The fix.** - **New `useHistoryEntryOwnership` hook** (`propr-ui/src/hooks/useHistoryEntryOwnership.ts`): it records which browser history entry the user is on when work starts. Later, the work only counts as "still here" if the page is mounted and the browser is on that same entry. It reads the browser's history, not React state. - **Autosave vs. leaving:** the autosave URL change from `/studio/new` to `/studio/` keeps the same history entry, so it still counts as "still here". Back, Forward, opening another draft or opening another page all fail the check. - **`useDraftCreation.ts`:** the check runs right after `generatePlan` returns. If the user has left, it skips the run hand-off and the navigation. The request is never cancelled or repeated. - **`PlanStudioPage.tsx` (same defect, one step earlier):** if the user left while the draft was still being created (`POST /drafts` pending), the in-place URL update overwrote whatever entry they were on, e.g. `/plans` became `/studio/`. It now uses the same check. This path wasn't in your reproduction; I fixed it because it's the same flow and the same defect. **Tests** - **Hook tests (`useDraftCreation.test.tsx`):** the acknowledgement is dropped both when the wizard unmounts and when it stays mounted but the browser has moved to another entry. The test harness now performs the real in-place URL update, so the existing tests prove the autosave path still navigates. - **Page tests (`PlanStudioPage.freshGenerate.test.tsx`)**, run under both BrowserRouter and native HashRouter: - Back, then the acknowledgement arrives: `/plans` stays, with no new history entry. - Opening another existing draft while the acknowledgement is pending: that draft is untouched and isn't shown as Generating. - Back while the draft is still being created: the `/plans` entry isn't rewritten. Each case then reopens the generated draft and checks Generating → Review with one create and one generation. - **Playwright (`planner-fresh-generate.pw.ts`):** your reproduction in all four layouts, then Forward to reopen and complete the run. These four failed against my first fix and pass now. The original four fresh-generate cases still pass. **Results** - Full UI unit suite: 3066/3066. - `tsc --noEmit` and `eslint --max-warnings 0` on `propr-ui`: clean. - Playwright fresh-generate file: 8/8. - With either check disabled, the new tests fail. HashRouter is covered by the jsdom page tests only, because the Playwright web build uses BrowserRouter. The `native-minimum` layout is still a BrowserRouter page with the native window frame added. I didn't create previews, because the result is the user staying on the page they chose, which a still image can't show. Permission, credential, provider and repository settings are unchanged. PR: #3325 Comment by: @integry (ID: 6354022697) Model: Claude Opus 5.5 --- propr-ui/e2e/planner-fresh-generate.pw.ts | 44 ++++++++- .../TaskPlanner/useDraftCreation.test.tsx | 38 ++++++- .../TaskPlanner/useDraftCreation.ts | 8 +- .../src/hooks/useHistoryEntryOwnership.ts | 34 +++++++ .../PlanStudioPage.freshGenerate.test.tsx | 99 ++++++++++++++++++- propr-ui/src/pages/PlanStudioPage.tsx | 11 ++- 6 files changed, 224 insertions(+), 10 deletions(-) create mode 100644 propr-ui/src/hooks/useHistoryEntryOwnership.ts diff --git a/propr-ui/e2e/planner-fresh-generate.pw.ts b/propr-ui/e2e/planner-fresh-generate.pw.ts index ac543b3e4..69b5be9f5 100644 --- a/propr-ui/e2e/planner-fresh-generate.pw.ts +++ b/propr-ui/e2e/planner-fresh-generate.pw.ts @@ -41,7 +41,7 @@ async function installNativeShell(page: Page) { * Reads carry a server-assigned name the create response lacks, so the document title shows when the page has applied * a read; the acknowledgement is held until then rather than for a fixed time. */ -async function routeFreshDraft(page: Page) { +async function routeFreshDraft(page: Page, options: { holdAcknowledgement?: Promise } = {}) { const server = { creations: 0, generations: 0, reads: 0, readAppliedBeforeAck: false, complete: () => {} }; const draft: Record = { draft_id: draftId, repository: 'integry/propr', initial_prompt: '', status: 'draft', attachments: [], chat_history: [], plan_json: [], @@ -70,8 +70,9 @@ async function routeFreshDraft(page: Page) { await page.route('**/api/planner/generate', async route => { server.generations++; expect(route.request().postDataJSON().draftId).toBe(draftId); + if (options.holdAcknowledgement) await options.holdAcknowledgement; // Barrier: the page has rendered the pre-generation read. On timeout the acknowledgement still goes out and the test's assertion fails. - server.readAppliedBeforeAck = await page.waitForFunction(name => document.title.includes(name), serverName, { timeout: 10_000 }) + else server.readAppliedBeforeAck = await page.waitForFunction(name => document.title.includes(name), serverName, { timeout: 10_000 }) .then(() => true, () => false); Object.assign(draft, { status: 'generating', generation_trace: { runId: 'run-1', steps: [{ name: 'relevance', status: 'in_progress' }] } }); await route.fulfill({ json: { success: true, runId: 'run-1', status: 'generating', message: 'started' } }); @@ -122,3 +123,42 @@ for (const layout of layouts) { expect(server.generations).toBe(1); }); } + +for (const layout of layouts) { + test(`leaving a fresh plan before its generation is acknowledged keeps the user where they went (${layout.name})`, async ({ page }) => { + await page.setViewportSize(layout.viewport); + if (layout.native) await installNativeShell(page); + await fixture(page); + let acknowledge!: () => void; + const server = await routeFreshDraft(page, { holdAcknowledgement: new Promise(resolve => { acknowledge = resolve; }) }); + await page.goto('/plans'); + await page.evaluate(() => { window.history.pushState(null, '', '/studio/new'); window.dispatchEvent(new PopStateEvent('popstate')); }); + + await expect(page.getByTestId('branch-chip')).toHaveAttribute('data-status', 'resolved'); + await page.getByPlaceholder(/Describe the feature/).fill(prompt); + const generate = page.getByRole('button', { name: /Generate Plan/ }); + await expect(generate).toBeEnabled(); + await generate.click(); + await expect.poll(() => server.generations).toBe(1); + await expect(page).toHaveURL(new RegExp(`/studio/${draftId}$`)); + + await page.goBack(); + await expect(page).toHaveURL(/\/plans$/); + const acknowledged = page.waitForResponse('**/api/planner/generate'); + acknowledge(); + await acknowledged; + // Give a late navigation every chance to run before asserting it did not. + await page.evaluate(() => new Promise(resolve => setTimeout(resolve, 500))); + await expect(page).toHaveURL(/\/plans$/); + await expect(page.getByPlaceholder(/Describe the feature/)).toHaveCount(0); + + // The run was not cancelled: reopening the draft shows its progress and then its Review, without generating again. + await page.goForward(); + await expect(page).toHaveURL(new RegExp(`/studio/${draftId}$`)); + await expect(page.getByText('Generating', { exact: true })).toBeVisible(); + server.complete(); + await expect(page.getByText(task.title).first()).toBeVisible({ timeout: 15_000 }); + expect(server.creations).toBe(1); + expect(server.generations).toBe(1); + }); +} diff --git a/propr-ui/src/components/TaskPlanner/useDraftCreation.test.tsx b/propr-ui/src/components/TaskPlanner/useDraftCreation.test.tsx index 475909932..0946092e9 100644 --- a/propr-ui/src/components/TaskPlanner/useDraftCreation.test.tsx +++ b/propr-ui/src/components/TaskPlanner/useDraftCreation.test.tsx @@ -21,8 +21,10 @@ function renderNewPlan(initialProps: HarnessProps) { const navigate = vi.fn(); const setError = vi.fn(); const setIsCreating = vi.fn(); - const onDraftCreatedInPlace = vi.fn(); + // As PlanStudioPage does: swap the visible path without a new history entry. + const onDraftCreatedInPlace = vi.fn((draft: PlannerDraft) => window.history.replaceState(window.history.state, '', `/studio/${draft.draft_id}`)); const onDraftCreated = vi.fn(); + const onGenerationStarted = vi.fn(); const flushPrompt = vi.fn((draftId: string, prompt: string) => updateDraft(draftId, { initial_prompt: prompt, name: prompt })); const hook = renderHook(({ config, localFiles }: HarnessProps) => { const autosave = useAutoDraftCreation({ @@ -31,11 +33,11 @@ function renderNewPlan(initialProps: HarnessProps) { }); const generate = useDraftCreation({ selectedRepo: 'integry/propr', config, localFiles, ensureDraftCreated: autosave.ensureDraftCreated, - uploadPendingLocalFiles: autosave.uploadPendingLocalFiles, flushPrompt, navigate, setError, setIsCreating, + uploadPendingLocalFiles: autosave.uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating, }); return { autosave, generate }; }, { initialProps }); - return { ...hook, navigate, setError, setIsCreating, onDraftCreatedInPlace, onDraftCreated }; + return { ...hook, navigate, setError, setIsCreating, onDraftCreatedInPlace, onDraftCreated, onGenerationStarted }; } const config = (overrides: Partial = {}): PlannerConfig => ({ ...baseConfig, prompt: 'Test prompt', baseBranch: 'develop', ...overrides }); @@ -44,6 +46,7 @@ describe('useDraftCreation', () => { beforeEach(() => { vi.resetAllMocks(); vi.useFakeTimers(); + window.history.replaceState({ key: 'studio', idx: 0 }, '', '/studio/new'); mockUpdateDraft.mockResolvedValue(undefined as never); mockUploadAttachment.mockImplementation(async (_draftId, file) => ({ id: `attachment-${file.name}`, originalName: file.name, tokenEstimate: 1 })); mockGeneratePlan.mockResolvedValue({ success: true, status: 'generating', message: 'started', runId: 'run-1' }); @@ -171,4 +174,33 @@ describe('useDraftCreation', () => { expect(mockCreateDraft).toHaveBeenCalledTimes(1); expect(mockGeneratePlan).toHaveBeenCalledTimes(1); }); + + // The run belongs to the server once acknowledged; leaving must neither cancel it nor let it pull the user back. + it.each([ + ['unmounts (back to the plans list)', (view: ReturnType) => view.unmount()], + // Still mounted: the router can keep rendering the studio while the next route's chunk loads. + ['stays mounted while the browser moves to another entry', () => window.history.pushState({ key: 'plans', idx: 1 }, '', '/plans')], + ])('drops the acknowledgement when the wizard %s while generation is pending', async (_label, leave) => { + mockCreateDraft.mockResolvedValue(makeDraft()); + const generation = createDeferred>>(); + mockGeneratePlan.mockReturnValueOnce(generation.promise); + const view = renderNewPlan({ config: config(), localFiles: [] }); + + let start!: Promise; + act(() => { start = view.result.current.generate(); }); + await act(async () => { await vi.advanceTimersByTimeAsync(0); }); + expect(mockGeneratePlan).toHaveBeenCalledTimes(1); + leave(view); + await act(async () => { + generation.resolve({ success: true, status: 'generating', message: 'started', runId: 'run-1' }); + await start; + await vi.advanceTimersByTimeAsync(5000); + }); + + expect(view.onGenerationStarted).not.toHaveBeenCalled(); + expect(view.navigate).not.toHaveBeenCalled(); + expect(view.setError).not.toHaveBeenCalledWith(expect.any(String)); + expect(mockCreateDraft).toHaveBeenCalledTimes(1); + expect(mockGeneratePlan).toHaveBeenCalledTimes(1); + }); }); diff --git a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts index 2ba7e72af..88e29a141 100644 --- a/propr-ui/src/components/TaskPlanner/useDraftCreation.ts +++ b/propr-ui/src/components/TaskPlanner/useDraftCreation.ts @@ -1,4 +1,5 @@ import { useCallback, useRef } from 'react'; +import { useHistoryEntryOwnership } from '../../hooks/useHistoryEntryOwnership'; import { generatePlan, PlannerDraft } from '../../api/proprApi'; import { getDraftConfigSnapshot, getPersistedDraftSettings, serializePersistedDraftSettings } from './setupWizardDraftConfig'; import { buildGenerationPayload, getDraftSetupSnapshot } from './setupWizardPayloads'; @@ -40,6 +41,7 @@ async function syncDraftSetup(draft: PlannerDraft, config: PlannerConfig): Promi // Generate in new mode: persist the draft through the same creation autosave uses, then start generation for it. export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating }: DraftCreationParams) { const isStartingRef = useRef(false); + const claimHistoryEntry = useHistoryEntryOwnership(); return useCallback(async () => { if (!selectedRepo || !config.prompt.trim()) return void setError('Please select a repository and enter a prompt'); if (!config.baseBranch) return void setError('Please wait for the repository branch to finish loading'); @@ -47,6 +49,7 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft isStartingRef.current = true; setIsCreating(true); setError(null); + const isStillOnEntry = claimHistoryEntry(); try { const draft = await ensureDraftCreated(); if (!draft) throw new Error('Failed to create draft'); @@ -55,6 +58,9 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft const baseBranchPersistenceWarning = await syncDraftSetup(draft, config); await uploadPendingLocalFiles(draft.draft_id, localFiles); const generation = await generatePlan(draft.draft_id, buildGenerationPayload(config)); + // After the user left, the run stays on the server and the draft reopens normally; the + // acknowledgement must not pull them back or replace whatever entry they are on now. + if (!isStillOnEntry(`/studio/${draft.draft_id}`)) return void setIsCreating(false); const draftWithPlan = constructDraftWithPlan({ ...draft, initial_prompt: prompt }, getDraftSetupSnapshot(config)); draftWithPlan.status = 'generating'; draftWithPlan.generation_trace = { steps: [], runId: generation.runId }; @@ -66,5 +72,5 @@ export function useDraftCreation({ selectedRepo, config, localFiles, ensureDraft } finally { isStartingRef.current = false; } - }, [selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, setError, setIsCreating]); + }, [selectedRepo, config, localFiles, ensureDraftCreated, uploadPendingLocalFiles, flushPrompt, onGenerationStarted, navigate, claimHistoryEntry, setError, setIsCreating]); } diff --git a/propr-ui/src/hooks/useHistoryEntryOwnership.ts b/propr-ui/src/hooks/useHistoryEntryOwnership.ts new file mode 100644 index 000000000..1b35576c9 --- /dev/null +++ b/propr-ui/src/hooks/useHistoryEntryOwnership.ts @@ -0,0 +1,34 @@ +import { useCallback, useEffect, useRef } from 'react'; +import { currentUiPathname } from '../config/runtimeMode'; + +interface HistoryEntry { key: unknown; idx: unknown; pathname: string } + +// The browser's current entry, not the router's: after Back the router may keep rendering the previous +// page inside a transition (lazy route chunk), but window history has already moved. +const currentHistoryEntry = (): HistoryEntry => { + const state = window.history.state as { key?: unknown; idx?: unknown } | null; + return { key: state?.key, idx: state?.idx, pathname: currentUiPathname() }; +}; + +/** + * Lets async work started on one history entry tell whether the user is still on it when it completes. + * claim() records the current entry; the returned check fails after unmount, back/forward or any + * navigation. replaceUiPathInPlace keeps the router's entry key and index, so the in-place + * /studio/new → /studio/:id swap stays owned when its path is passed as allowedPathname. + */ +export function useHistoryEntryOwnership() { + const mountedRef = useRef(true); + useEffect(() => { + mountedRef.current = true; + return () => { mountedRef.current = false; }; + }, []); + return useCallback(() => { + const claimed = currentHistoryEntry(); + return (allowedPathname?: string) => { + if (!mountedRef.current) return false; + const current = currentHistoryEntry(); + return current.key === claimed.key && current.idx === claimed.idx + && (current.pathname === claimed.pathname || current.pathname === allowedPathname); + }; + }, []); +} diff --git a/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx b/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx index d9d3c597c..ca5e8239c 100644 --- a/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx +++ b/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx @@ -1,5 +1,5 @@ import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; -import { BrowserRouter, HashRouter, Route, Routes } from 'react-router-dom'; +import { BrowserRouter, HashRouter, Route, Routes, useNavigate, type NavigateFunction } from 'react-router-dom'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { DraftUpdatePayload } from '@propr/shared'; import PlanStudioPage from './PlanStudioPage'; @@ -66,6 +66,13 @@ const emit = (payload: Partial) => act(() => { for (const listener of socket.listeners) listener({ draftId: 'draft-1', timestamp: new Date().toISOString(), ...payload } as DraftUpdatePayload); }); +// In-app navigation as a sidebar link would perform it. +let appNavigate: NavigateFunction; +function NavigationProbe() { + appNavigate = useNavigate(); + return null; +} + // Mirrors App.tsx: both studio routes render the same element tree, so the page instance survives /studio/new → /studio/:id. function renderStudio(desktop: boolean) { vi.stubGlobal('__PROPR_DESKTOP__', desktop); @@ -76,6 +83,7 @@ function renderStudio(desktop: boolean) { return render( + } /> } /> @@ -93,7 +101,8 @@ const historyStep = (step: () => void) => act(async () => { step(); await popped; }); -const studioUrl = (desktop: boolean) => (desktop ? `${renderer}#/studio/draft-1` : 'https://app.example/studio/draft-1'); +const appUrl = (desktop: boolean, path: string) => (desktop ? `${renderer}#${path}` : `https://app.example${path}`); +const studioUrl = (desktop: boolean) => appUrl(desktop, '/studio/draft-1'); async function enterPrompt() { const textarea = await screen.findByPlaceholderText(/Describe the feature/); @@ -202,4 +211,90 @@ describe.each([false, true])('fresh plan generation becomes the displayed draft expect(screen.queryByText('Generating')).not.toBeInTheDocument(); expect(createDraft).toHaveBeenCalledTimes(1); }); + + describe('after the user leaves a pending fresh plan', () => { + const otherDraft = { ...serverDraft('draft'), draft_id: 'other-draft', initial_prompt: 'Rename the settings page' } as PlannerDraft; + let generation: ReturnType>>>; + let serverStatus: string; + beforeEach(() => { + serverStatus = 'generating'; + vi.mocked(getDraft).mockImplementation(async id => (id === 'other-draft' ? otherDraft : serverDraft(serverStatus, 'run-1'))); + generation = deferred(); + vi.mocked(generatePlan).mockImplementationOnce(() => generation.promise); + }); + const acknowledge = () => act(async () => generation.resolve({ success: true, status: 'generating', message: 'started', runId: 'run-1' })); + const expectSingleRun = async () => { + await waitPastAutosaveDebounce(); + expect(createDraft).toHaveBeenCalledTimes(1); + expect(generatePlan).toHaveBeenCalledTimes(1); + }; + const reopenGeneratedDraft = async () => { + await act(async () => appNavigate('/studio/draft-1')); + expect(await screen.findByText('Generating')).toBeInTheDocument(); + serverStatus = 'review'; + await emit({ runId: 'run-1', draftStatus: 'review' }); + expect(await screen.findByTestId('review')).toHaveTextContent('draft-1:review'); + expect(window.location.href).toBe(studioUrl(desktop)); + }; + + it('keeps the plans list when the acknowledgement arrives after Back', async () => { + renderStudio(desktop); + const historyLength = window.history.length; + await enterPrompt(); + fireEvent.click(generateButton()); + await waitFor(() => expect(generatePlan).toHaveBeenCalledTimes(1)); + expect(window.location.href).toBe(studioUrl(desktop)); + + await historyStep(() => window.history.back()); + expect(await screen.findByRole('heading', { name: 'Plans' })).toBeInTheDocument(); + await acknowledge(); + + expect(screen.getByRole('heading', { name: 'Plans' })).toBeInTheDocument(); + expect(window.location.href).toBe(appUrl(desktop, '/plans')); + expect(window.history.length).toBe(historyLength); + await expectSingleRun(); + expect(window.location.href).toBe(appUrl(desktop, '/plans')); + await reopenGeneratedDraft(); + expect(generatePlan).toHaveBeenCalledTimes(1); + }); + + it('keeps another opened draft untouched when the acknowledgement arrives', async () => { + renderStudio(desktop); + await enterPrompt(); + fireEvent.click(generateButton()); + await waitFor(() => expect(generatePlan).toHaveBeenCalledTimes(1)); + + await act(async () => appNavigate('/studio/other-draft')); + expect(await screen.findByDisplayValue('Rename the settings page')).toBeInTheDocument(); + await acknowledge(); + + expect(window.location.href).toBe(appUrl(desktop, '/studio/other-draft')); + expect(screen.getByDisplayValue('Rename the settings page')).toBeInTheDocument(); + expect(screen.queryByText('Generating')).not.toBeInTheDocument(); + await expectSingleRun(); + expect(window.location.href).toBe(appUrl(desktop, '/studio/other-draft')); + await reopenGeneratedDraft(); + expect(generatePlan).toHaveBeenCalledTimes(1); + }); + + it('does not rewrite the plans entry when the draft is created after Back', async () => { + const creation = deferred(); + vi.mocked(createDraft).mockImplementationOnce(() => creation.promise); + renderStudio(desktop); + await enterPrompt(); + fireEvent.click(generateButton()); + await waitFor(() => expect(createDraft).toHaveBeenCalledTimes(1)); + + await historyStep(() => window.history.back()); + expect(await screen.findByRole('heading', { name: 'Plans' })).toBeInTheDocument(); + await act(async () => creation.resolve(serverDraft('draft'))); + await waitFor(() => expect(generatePlan).toHaveBeenCalledTimes(1)); + await acknowledge(); + + expect(window.location.href).toBe(appUrl(desktop, '/plans')); + expect(screen.getByRole('heading', { name: 'Plans' })).toBeInTheDocument(); + await expectSingleRun(); + await reopenGeneratedDraft(); + }); + }); }); diff --git a/propr-ui/src/pages/PlanStudioPage.tsx b/propr-ui/src/pages/PlanStudioPage.tsx index 962dfa4c8..f0482f295 100644 --- a/propr-ui/src/pages/PlanStudioPage.tsx +++ b/propr-ui/src/pages/PlanStudioPage.tsx @@ -1,8 +1,9 @@ -import React, { useState, useCallback, useEffect } from 'react'; +import React, { useState, useCallback, useEffect, useMemo } from 'react'; import { useParams, Link, useLocation, useNavigate } from 'react-router-dom'; import { motion } from 'framer-motion'; import { useDocumentTitle } from '../hooks/useDocumentTitle'; import { replaceUiPathInPlace } from '../config/runtimeMode'; +import { useHistoryEntryOwnership } from '../hooks/useHistoryEntryOwnership'; import { useDraft } from '../hooks/useDraft'; import { useGenerationPolling } from '../hooks/useGenerationPolling'; import SetupWizard from '../components/TaskPlanner/SetupWizard'; @@ -394,11 +395,17 @@ const PlanStudioPage: React.FC = ({ isNew = false }) => { // Handle draft created in-place (auto-save in new mode) // This updates the URL without navigation, preserving focus and avoiding flicker + // A creation that resolves after the user left must not rewrite the entry they are on now. + const claimHistoryEntry = useHistoryEntryOwnership(); + // Re-claimed per router entry: NewDraftView remounts on location.key, starting a new creation. + // eslint-disable-next-line react-hooks/exhaustive-deps + const isStillOnEntry = useMemo(() => claimHistoryEntry(), [claimHistoryEntry, location.key]); const handleDraftCreatedInPlace = useCallback((draft: PlannerDraft) => { + if (!isStillOnEntry()) return; setInPlaceDraft(draft); // Update URL without triggering navigation - this keeps the component mounted replaceUiPathInPlace(`/studio/${draft.draft_id}`); - }, []); + }, [isStillOnEntry]); // Determine the effective draft ID for useDraft // When in new mode with an in-place draft, use that draft's ID From b663944f7f162c29c2139450a2d27f869896d9cd Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:42:57 +0000 Subject: [PATCH 5/5] =?UTF-8?q?feat(ai):=20I=20fixed=20F2.=20A=20late=20re?= =?UTF-8?q?ad=20for=20draft=20A=20=E2=80=94=20whether=20it=20succeeds,=20f?= =?UTF-8?q?ails,=20or=20finishes=20loading=20=E2=80=94=20can=20no=20longer?= =?UTF-8?q?=20change=20draft=20B,=20its=20active=20run,=20or=20a=20newer?= =?UTF-8?q?=20read=20of=20A=20after=20you=20come=20back=20to=20it.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I fixed F2. A late read for draft A — whether it succeeds, fails, or finishes loading — can no longer change draft B, its active run, or a newer read of A after you come back to it. **Fix** (`propr-ui/src/hooks/useDraft.ts`): - Each read of a draft now gets a sequence number. Changing `draftId` or unmounting invalidates every read still in flight. - A read only updates the draft, the active run ID or the error if it is still the newest read and its draft is still selected. That second check is what covers A → B → A: the ID matches again, but the old read is no longer the newest. - Loading is cleared by the newest read for the current draft, so a late A read can't end B's loading state. If you go to `/studio/new` (nothing selected), an abandoned read still clears loading, as it did before. - `fetchDraft` won't start for a draft that is no longer selected. Otherwise a `refetch` callback held from A's render could start an A read after switching to B, and that read would outrank B's. - The existing guard that stops a pre-acknowledgement read from replacing a just-started run is unchanged. The draft/run checks on the acknowledgement path (F1) and the history-entry navigation checks (S3) aren't touched. Nothing cancels or repeats a generation request. One behaviour change for the same draft: if two reads overlap, only the newest result is used. Before, whichever finished last won. **New tests** (`propr-ui/src/hooks/useDraft.selection.test.tsx`, 7 cases). They live in their own file because adding them to `useDraft.test.tsx` broke the 400-line lint limit. Each starts by reopening A from a history entry whose saved state says "generating", with A's background read held open: - B loads, then A's read resolves: B and its run stay selected, B's live updates are still accepted and updates carrying A's run ID are ignored. - A's read resolves while B is still loading: B stays loading, then B is shown. - A's read fails while B is loading: no error appears on B and B stays loading. - A → B → A, then the obsolete A read resolves or fails (two cases): A's newer result stays. - A → B → A, where the obsolete read finishes before the new A read: loading stays on until the new read lands. - A `refetch` held from A's render is called after switching to B: no read starts and B stays. **Verification:** - All 7 new tests fail against the previous `useDraft.ts` and pass with the fix. - `useDraft.test.tsx` is byte-identical to HEAD. - Full UI unit suite passes (3073/3073). I ran it before moving the tests into their own file; after the move, `useDraft*`, `PlanStudioPage*` and `TaskPlanner` pass (17 and 299 tests). `tsc --noEmit` and `eslint --max-warnings 0` on `src/hooks/` are clean. - `planner-fresh-generate.pw.ts` passes in real Chromium in all 4 layouts (8/8), covering the original fresh-generation flow and leaving before the acknowledgement arrives. I haven't tried it in the installed native app; your independent diagnostic and signed-native validation are still separate. I made no preview images, since nothing visible changes beyond the wrong-draft state being gone. PR: #3325 Comment by: @integry (ID: 6354464426) Model: Claude Opus 5.5 --- .../src/hooks/useDraft.selection.test.tsx | 172 ++++++++++++++++++ propr-ui/src/hooks/useDraft.ts | 19 +- 2 files changed, 187 insertions(+), 4 deletions(-) create mode 100644 propr-ui/src/hooks/useDraft.selection.test.tsx diff --git a/propr-ui/src/hooks/useDraft.selection.test.tsx b/propr-ui/src/hooks/useDraft.selection.test.tsx new file mode 100644 index 000000000..6314cb640 --- /dev/null +++ b/propr-ui/src/hooks/useDraft.selection.test.tsx @@ -0,0 +1,172 @@ +import { renderHook, act } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { useDraft } from './useDraft'; +import { getDraft } from '../api/proprApi'; +import type { DraftUpdatePayload } from '@propr/shared'; + +// Reads are owned by the selection that requested them; see useDraft.test.tsx for the +// generation-acknowledgement and socket cases. +const draftUpdateListeners = new Set<(payload: DraftUpdatePayload) => void | Promise>(); +const socketState = { + isConnected: true, + subscribeToDraft: vi.fn(), + unsubscribeFromDraft: vi.fn(), + onDraftUpdate: vi.fn((callback: (payload: DraftUpdatePayload) => void | Promise) => { + draftUpdateListeners.add(callback); + return () => { + draftUpdateListeners.delete(callback); + }; + }), +}; + +vi.mock('../api/proprApi', () => ({ + getDraft: vi.fn(), +})); + +vi.mock('../contexts/useSocket', () => ({ + useSocket: () => socketState, +})); + +const mockGetDraft = vi.mocked(getDraft); + +function createDeferred() { + let resolve!: (value: T) => void; + let reject!: (reason: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +beforeEach(() => { + vi.clearAllMocks(); + mockGetDraft.mockReset(); + draftUpdateListeners.clear(); +}); + +describe('useDraft when the selection changes while a read is in flight', () => { + type DraftFetch = ReturnType>>>; + const draftRecord = (id: string, status: string, runId?: string) => ({ + draft_id: id, repository: 'integry/propr', initial_prompt: `Prompt ${id}`, status, attachments: [], + created_at: '2026-10-22T22:00:00Z', ...(runId ? { generation_trace: { runId, steps: [] } } : {}), + }); + const queueFetch = (): DraftFetch => { + const fetch = createDeferred>>(); + mockGetDraft.mockImplementationOnce(() => fetch.promise); + return fetch; + }; + // Revisiting A through a history entry whose router snapshot still says generating. + const renderRevisitedGeneratingA = () => { + const resyncA = queueFetch(); + const initialData = draftRecord('draft-a', 'generating', 'generation-run-a') as never; + const hook = renderHook(({ id }) => useDraft(id, { initialData }), { initialProps: { id: 'draft-a' } }); + expect(mockGetDraft).toHaveBeenLastCalledWith('draft-a'); + return { ...hook, resyncA }; + }; + + it('keeps B and its run when A’s background resync resolves after B loaded', async () => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + expect(result.current.loading).toBe(true); + + await act(async () => fetchB.resolve(draftRecord('draft-b', 'generating', 'generation-run-b') as never)); + expect(result.current.loading).toBe(false); + await act(async () => resyncA.resolve(draftRecord('draft-a', 'review', 'generation-run-a') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', status: 'generating', generation_trace: { runId: 'generation-run-b' } }); + expect(result.current.loading).toBe(false); + expect(result.current.error).toBeNull(); + + // B's active run identity survived: its socket progress is still accepted, A's is not. + await act(async () => { + for (const listener of draftUpdateListeners) { + await listener({ draftId: 'draft-b', runId: 'generation-run-a', draftStatus: 'generating', timestamp: '2026-10-22T22:01:00Z', + generationTrace: { runId: 'generation-run-a', steps: [{ name: 'context', status: 'failed' }] } } as never); + await listener({ draftId: 'draft-b', runId: 'generation-run-b', draftStatus: 'generating', timestamp: '2026-10-22T22:01:01Z', + generationTrace: { runId: 'generation-run-b', steps: [{ name: 'context', status: 'in_progress' }] } } as never); + } + }); + expect(result.current.draft?.generation_trace?.steps).toEqual([{ name: 'context', status: 'in_progress' }]); + expect(mockGetDraft).toHaveBeenCalledTimes(2); + }); + + it('keeps B loading when A’s resync settles before B’s read, then installs B', async () => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + + await act(async () => resyncA.resolve(draftRecord('draft-a', 'review', 'generation-run-a') as never)); + expect(result.current.loading).toBe(true); + expect(result.current.draft).toBeNull(); + + await act(async () => fetchB.resolve(draftRecord('draft-b', 'review', 'generation-run-b') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', status: 'review', generation_trace: { runId: 'generation-run-b' } }); + expect(result.current.loading).toBe(false); + }); + + it('does not report A’s failed resync on B, nor end B’s loading', async () => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + + await act(async () => resyncA.reject(new Error('draft-a unavailable'))); + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(true); + + await act(async () => fetchB.resolve(draftRecord('draft-b', 'generating', 'generation-run-b') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', generation_trace: { runId: 'generation-run-b' } }); + expect(result.current.error).toBeNull(); + }); + + it.each([ + ['resolves', (fetch: DraftFetch) => fetch.resolve(draftRecord('draft-a', 'generating', 'generation-run-a') as never)], + ['fails', (fetch: DraftFetch) => fetch.reject(new Error('stale read failed'))], + ])('ignores A’s obsolete resync that %s after re-selecting A via B', async (_label, settleObsolete) => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + await act(async () => fetchB.resolve(draftRecord('draft-b', 'review', 'generation-run-b') as never)); + + const refetchA = queueFetch(); + rerender({ id: 'draft-a' }); + await act(async () => refetchA.resolve(draftRecord('draft-a', 'review', 'generation-run-a2') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-a', status: 'review', generation_trace: { runId: 'generation-run-a2' } }); + + await act(async () => settleObsolete(resyncA)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-a', status: 'review', generation_trace: { runId: 'generation-run-a2' } }); + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(false); + expect(mockGetDraft).toHaveBeenCalledTimes(3); + }); + + it('lets the newest read for the re-selected draft own loading even if the obsolete one settles first', async () => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + await act(async () => fetchB.resolve(draftRecord('draft-b', 'review', 'generation-run-b') as never)); + const refetchA = queueFetch(); + rerender({ id: 'draft-a' }); + + await act(async () => resyncA.resolve(draftRecord('draft-a', 'generating', 'generation-run-a') as never)); + expect(result.current.loading).toBe(true); + expect(result.current.draft).toBeNull(); + + await act(async () => refetchA.resolve(draftRecord('draft-a', 'review', 'generation-run-a2') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-a', status: 'review', generation_trace: { runId: 'generation-run-a2' } }); + expect(result.current.loading).toBe(false); + }); + + it('ignores a refetch requested through A’s stale callback after switching to B', async () => { + const { result, rerender, resyncA } = renderRevisitedGeneratingA(); + await act(async () => resyncA.resolve(draftRecord('draft-a', 'generating', 'generation-run-a') as never)); + const refetchFromA = result.current.refetch; + const fetchB = queueFetch(); + rerender({ id: 'draft-b' }); + + await act(async () => refetchFromA()); + await act(async () => fetchB.resolve(draftRecord('draft-b', 'review', 'generation-run-b') as never)); + expect(result.current.draft).toMatchObject({ draft_id: 'draft-b', generation_trace: { runId: 'generation-run-b' } }); + expect(mockGetDraft).toHaveBeenCalledTimes(2); + }); +}); diff --git a/propr-ui/src/hooks/useDraft.ts b/propr-ui/src/hooks/useDraft.ts index 17397e154..4ae2f2179 100644 --- a/propr-ui/src/hooks/useDraft.ts +++ b/propr-ui/src/hooks/useDraft.ts @@ -101,31 +101,42 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra // the pre-generation snapshot and must not replace the acknowledged run. const activationSeqRef = useRef(0); + // Each read is owned by the selection that requested it: changing draftId or unmounting + // invalidates every read in flight, and a newer read for the same draft supersedes an older one, + // so a late completion cannot alter a newer selection even when the draft ID matches again. + const fetchSeqRef = useRef(0); + // Fetch function - only sets loading for explicit refetch, not background refresh const fetchDraft = useCallback(async (showLoading = true) => { - if (!draftId) return; + if (!draftId || draftId !== draftIdRef.current) return; const activationSeq = activationSeqRef.current; const isSuperseded = () => activationSeq !== activationSeqRef.current; + const fetchSeq = ++fetchSeqRef.current; + const isCurrent = () => fetchSeq === fetchSeqRef.current && draftIdRef.current === draftId; try { if (showLoading) { setLoading(true); } setError(null); const data = await getDraft(draftId); - if (isSuperseded()) return; + if (!isCurrent() || isSuperseded()) return; const parsedDraft = parseJsonFields(data as unknown as Record) as unknown as PlannerDraft; activeRunIdRef.current = parsedDraft.generation_trace?.runId; setDraft(parsedDraft); } catch (err) { - if (!isSuperseded()) setError((err as Error).message || 'Failed to fetch draft'); + if (isCurrent() && !isSuperseded()) setError((err as Error).message || 'Failed to fetch draft'); } finally { - if (showLoading) { + // The latest read ends loading for its selection, including a loading read it superseded; + // a read abandoned for /studio/new (no selection) still ends its own loading state. + if (isCurrent() || (showLoading && !draftIdRef.current)) { setLoading(false); } } }, [draftId]); + useEffect(() => () => { fetchSeqRef.current += 1; }, [draftId]); + // Initial fetch effect - handles both initial load and draftId changes useEffect(() => { // If we already have draft data (from initialData), skip the loading fetch