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..69b5be9f5 --- /dev/null +++ b/propr-ui/e2e/planner-fresh-generate.pw.ts @@ -0,0 +1,164 @@ +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 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 = [ + { 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. + * + * 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, 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: [], + created_at: '2026-10-22T22:00:00Z', context_config: { baseBranch: 'main' }, + }; + 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 }); + } + 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'] } })); + 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); + 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. + 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' } }); + }); + 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.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); + 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); + }); +} + +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/SetupWizard.tsx b/propr-ui/src/components/TaskPlanner/SetupWizard.tsx index b2e872155..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) => void; + onGenerationStarted?: (runId: string, draftId: 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..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(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 0f77a78ab..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) => 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); + 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.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 176f142bd..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'; @@ -14,6 +15,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, draftId: string, snapshot?: PlannerDraft) => void; navigate: (path: string, options?: { replace?: boolean; state?: unknown }) => void; setError: React.Dispatch>; setIsCreating: React.Dispatch>; @@ -35,8 +39,9 @@ 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); + 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'); @@ -44,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'); @@ -52,9 +58,13 @@ 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 }; + 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'); @@ -62,5 +72,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, claimHistoryEntry, setError, setIsCreating]); } 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.test.tsx b/propr-ui/src/hooks/useDraft.test.tsx index daf6d92c4..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({ @@ -245,6 +245,102 @@ 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', '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'); + 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('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-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?.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 () => { const deferredFetch = createDeferred>>(); diff --git a/propr-ui/src/hooks/useDraft.ts b/propr-ui/src/hooks/useDraft.ts index d06d6e24e..4ae2f2179 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, draftId: string, snapshot?: PlannerDraft) => void; } function ensureArray(value: unknown): unknown[] { @@ -94,34 +94,57 @@ 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); + + // 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 (!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) { - 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 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 +178,28 @@ export const useDraft = (draftId: string, options: UseDraftOptions = {}): UseDra }); }, []); - const activateGenerationRun = useCallback((runId: string) => { + // 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 || runDraftId !== currentDraftId) return; + if (snapshot && snapshot.draft_id !== currentDraftId) return; activeRunIdRef.current = runId; + if (snapshot) activationSeqRef.current += 1; setDraft(currentDraft => { - if (!currentDraft || currentDraft.draft_id !== draftId) return currentDraft; + const base = currentDraft?.draft_id === currentDraftId ? currentDraft : snapshot; + if (!base) return currentDraft; return { - ...currentDraft, + ...base, status: 'generating', generation_trace: { steps: [], runId }, }; }); - }, [draftId]); + if (snapshot) { + setError(null); + setLoading(false); + } + }, []); // Handle draft update from WebSocket const handleDraftUpdate = useCallback(async (payload: DraftUpdatePayload) => { 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 new file mode 100644 index 000000000..ca5e8239c --- /dev/null +++ b/propr-ui/src/pages/PlanStudioPage.freshGenerate.test.tsx @@ -0,0 +1,300 @@ +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +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'; +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); +}); + +// 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); + // 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 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/); + 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); + }); + + 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 758e03ec9..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'; @@ -197,7 +198,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, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void }> = ({ currentStage, draft, onRefetch, onGenerationStarted, onDraftMetadataPersisted }) => ( void; onRefetch?: () => void; - onGenerationStarted?: (runId: string) => void; + onGenerationStarted?: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted?: (update: PromptPersistedUpdate) => void; }> = ({ draft, onDraftCreated, onRefetch, onGenerationStarted, onDraftMetadataPersisted, singleTask }) => ( void; - onGenerationStarted: (runId: string) => void; + onGenerationStarted: (runId: string, draftId: string, snapshot?: PlannerDraft) => void; onDraftMetadataPersisted: (update: PromptPersistedUpdate) => void; } @@ -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