Guard setup inputs on the real project load error and hydration state

[christophervoelpel]

Summary

Keep Setup inputs closed until a full project load succeeds or the project is created locally (SM-10). Use the recorded load-error state rather than the resource error that the loader catches internally.

When a full load returns 404 while a local editor save is unsettled, retain the editor copy without synthesizing blank inputs or issuing a full replacement PATCH. Setup now shows recovery controls instead of an indefinite spinner. Retry supports a transient failure; Back to projects provides an exit for a deleted project. This recovery state is limited to Setup and does not replace the other editors with an error screen.

Validation

• Setup and ConfigService focused suites: 90 passed; lint and spec typecheck passed.
• Recovery assertions failed before the follow-up fix and passed after it.
• Tests cover pre-load/pending states, GET404 plus pending PATCH404, no blank inputs or full PATCH, normal hydration, local creation and retry success.
• Chrome at 1280x1169, 100% zoom: rendered failure, Retry and keyboard navigation home passed with the local backend unavailable. The double-404 race is covered by HTTP tests, not a live cloud check.

Stacked on #199. Its updated parent is included; merge after #199. Rebase and retarget to main after the parent squash merge for fresh application CI.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-typescript-approvers -
  Can grant: [echom]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-typescript-approvers -
  Can grant: [victor-paunescu]

[christophervoelpel]

PR #202 — P2 — settled unhydrated state remains “Loading project…”

Reviewed head: b1972ffe1dfa8b439c93569bfc1fca236d3e8232.

The hydration gate correctly prevents a destructive blank-input save, but the full-load 404/local-copy branch at config.ts:990–995 leaves Setup in loading=false, error=false, loaded=false. setup.html displays an indefinite spinner instead of a project-load recovery action.

A reachable case is shared-project deletion: a user edits an existing project, an editor save is pending, another admitted user deletes the project, and navigation to Setup plus the pending PATCH both return 404. The save-error snackbar retries the PATCH, not the Setup load. A probe using the real ConfigService and Setup DOM confirms the spinner remains after both requests settle. The broader GET-404/PATCH-200 example was not relied on as production evidence.

Please keep the hydration safety gate and expose an explicit current-view error/deleted-project state with recovery/navigation. Acceptance: this sequence leaves the loading state, preserves unsaved local data as appropriate, and never synthesizes blank inputConfig or sends a full replacement before hydration. Reuse existing recovery UI; no new retry/save framework is needed.

[christophervoelpel]

Review: merge after small fixes — one two-line template change closes the remaining gap

Multi-agent review (reviewer → independent critique agent re-verifying each claim against the code). The critique pass rejected the reviewer's proposed fix and found a cheaper one; details below.

The guard is correct and matches DEVELOPING.md's "absent inputConfig means not loaded". Regression value verified by tracing the old computed: the "projectLoadError set even if id matches" test and both 404-with-in-flight-autosave tests genuinely fail against pre-PR code, and the isCurrentLoad() guard correctly prevents a stale load from marking hydration. The body also documents the rejected alternative (projectLoadErrorValue.set in the 404-local branch) for exactly the right reason.

Important

In the scenario this PR fixes, the user now gets a permanent spinner

The 404-with-local-copy branch (config.ts:976-981) returns latestSource without calling projectLoadErrorValue.set(error) and without rethrowing. So resource.error() stays undefined, and:

• setupInputsError() → false
• setupInputsLoading() → false
• setupInputsHydrated() → false

setup.html:24 then renders the @else if (setupLoading() || !setupReady()) branch — "Loading project…" forever, with no Retry — and nothing re-triggers a load when the save settles. DEVELOPING.md:249 promises that "Setup waits for the full load and offers Retry on failure before enabling its form."

This is still strictly better than the old behaviour (blank inputs + a full PATCH that wipes server state), so it's not a merge blocker. But the fix is small enough to land here.

Cheapest correct fix — template-only, in setup.html, no service change:

@if (setupError() || (!setupLoading() && !setupReady())) { …Retry… }
@else if (setupLoading())                                { …spinner… }
@else                                                    { …form… }

Two edited lines, and it gives the Retry affordance DEVELOPING.md already promises. Smaller than the follow-up issue would be.

Suggested deletions (~26 lines)

1. The vacuous patch assertion in the new "404 with in-flight autosave" test:

   expect(httpClientMock.patch).not.toHaveBeenCalledWith('/api/projects/proj-hydrate', expect.anything())

   In a service-only TestBed nothing synthesizes inputConfig, so this can't fail. The real proof is the setup.spec.ts test. Delete the 4 lines.

2. "navigates home and resets project on 404 without local copy" — never asserts navigation (so the name lies), and it's a pure control that passes against old code. Either assert routerMock.navigate was called with ['/'], or delete it (~22 lines).

Keep the "normal successful load" control — that one guards against over-guarding, which would strand every user on the spinner. And keep the (service as any).projectLoadErrorValue.set(...) poke: it's consistent with the file's existing markPersisted helper, and it's the only test that fails against old code for the dead-guard half.

Nit: three unreachable setupInputsHydrated writes

config.ts:942, :1446, :1466 look dead. setupInputsLoaded only consults the flag via isHydratedServerProject, which requires value().id === projectId() with projectId() non-null. In setNewProject the fresh uuid can never equal a non-null projectId() (that path doesn't touch projectId), and after the reset at :1443 value().id is ''. Keep only :951 (false on load start) and :966 (true on successful full GET).

Correcting the first pass: :1466 set(true) and :942 set(false) do not race observably — on both paths setupInputsLoaded is decided by isLocalNewProject or by ''-vs-null, never by the flag. So this is a 3-line cleanup with zero behaviour change, not a correctness item. (Derived by reading loadProjectConfig, not by running the suite.)

Explicitly not recommended

• Don't add a setupInputsUnavailable service signal. The template reorder above does the same job with no new state.
• Especially don't set projectLoadErrorValue in the 404-with-local branch. It's the obvious one-liner and it's actively wrong: projectLoadError() also gates storyboard.html:17, composition.html:17 and output-video.html:17, so it would blow away the whole app UI this PR works to preserve. Your PR body already reasons this out correctly.

Process notes

• This PR has run no tests. It targets fix/archived-candidate-never-renders rather than main, and all three workflows trigger on pull_request: branches: ["main"] — gh pr checks shows only cla/google. Same for Validate model-returned product and image ids #201 and Treat an absent storyboard key as no change #203. Worth fixing the trigger (branches: ['**']) rather than the PRs.
• Stacking with Exclude archived candidates from the rendered video #199 verified clean. Exclude archived candidates from the rendered video #199 touches only resolveSceneRenderClip (~config.ts:464-472); this PR touches 639, 939-1040 and 1443-1466. Base commit e72d24d is Exclude archived candidates from the rendered video #199's head, so a rebase onto main after Exclude archived candidates from the rendered video #199 lands is textually clean. Only risk: if Exclude archived candidates from the rendered video #199 is squash-merged, retarget this to main or it will re-present Exclude archived candidates from the rendered video #199's commit.

[christophervoelpel]

Consolidated Review

Verdict: Fix one blocker, then merge. This note reconciles the 19:29 and 19:50 comments against head b1972ff; every item below was re-verified by tracing callers in config.ts and setup.html and by running the tests and probes under Evidence.

Do before merge

1. Give the settled-but-unhydrated 404-with-local-copy branch a real terminal outcome. config.ts:990-999 returns the local copy without setting projectLoadErrorValue and without rethrowing, so setupInputsHydrated never flips true and setup.html:17-28 renders the plain spinner forever, with no Retry. Reachable case: a shared project is deleted while an editor save is pending, both the Setup GET and the PATCH return 404, and the user is stuck. Acceptance: the sequence ends in an explicit recovery state, never an indefinite spinner, and still never synthesizes a blank inputConfig or a full replacement before hydration.
   • (a) Do not simply wire the existing Retry to this state: reloadProjectConfig() (config.ts:1530-1532) reruns the loader with the same params and localProjectAtLoad survives, so the same branch fires again and the user loops. The fix must clear that condition or offer navigation home.
   • (b) The two-line setup.html condition proposed in the 19:50 comment (setupError() || (!setupLoading() && !setupReady())) is the right direction but needs a "load was requested for this route" guard: a service-level probe constructing ConfigService without calling loadProjectConfig shows both signals false before any load starts, so the bare condition would show the error block on ordinary navigation until the load kicks in.

Optional, does not block

• Delete the vacuous not.toHaveBeenCalledWith('/api/projects/proj-hydrate', ...) assertion in the 404-with-in-flight-autosave test in config-mediated.spec.ts. The payload is always editor-scope, so the PATCH always targets /editor, and the assertion cannot fail regardless of the guard.
• Fix or rename the "navigates home and resets project on 404 without local copy" test; it never asserts routerMock.navigate.
• Drop the two dead setupInputsHydrated writes at config.ts:942 and :1446; keep :951, :966, :1466. Zero behavior change, confirmed by removing them and rerunning the suite.

Rejected or superseded, do not re-litigate

• The 19:50 comment's claim that the template swap closes the gap in two lines on its own: superseded by sub-bullet (b) above.
• The 19:50 comment's "explicitly not recommended" items stand: no new setupInputsUnavailable signal, and do not set projectLoadErrorValue in the 404-local branch, since that flag also gates storyboard.html:17, composition.html:17, output-video.html:17 and would blow away the rest of the app UI.
• CI showing only cla/google here is not a code defect; it follows from targeting the #199 branch, resolved by the rebase step below.

Evidence

• npm test -- --watch=false --include src/app/setup/setup.spec.ts --include src/app/services/config/config-mediated.spec.ts: 2 files, 89 tests passed, lint clean.
• Removed the two dead setupInputsHydrated writes, reran every spec touching this state: 93 tests passed.
• Fresh TestBed probe constructing ConfigService without calling loadProjectConfig: the proposed condition evaluates true one tick after construction, before any real load, confirming the false positive.
• Not run: live browser confirmation; no application CI at this head, since the PR targets #199, not main.

Way forward

• Suggested order for the batch: Use an explicit MIME allow-list and support AVIF #196, Clear video source on resolve failure and keep cached blobs #205, Validate video duration limits and audio parameter types #204, Exclude archived candidates from the rendered video #199 and Clamp clip duration to the footage remaining after skip_time #200 first (independent, ready or one small fix each), then Preserve deployed env values on redeploy and accept quoted config #197 and Remove the duplicate GCS signing cache from orch.py #198 once their fixes land, then the stacked children Validate model-returned product and image ids #201 (on Use an explicit MIME allow-list and support AVIF #196), Treat an absent storyboard key as no change #203 (on Remove the duplicate GCS signing cache from orch.py #198) and Guard setup inputs on the real project load error and hydration state #202 (on Exclude archived candidates from the rendered video #199), each rebased onto main after its parent squash-merges and retargeted to main so CI runs.
• Base is #199 (e72d24d), diff clean against it.
• Confirm manually the chosen fix matches intended behavior for a deleted project versus a transient 404, since both take the same code path today.

[christophervoelpel]

Follow-up on Consolidated Review

I addressed the Setup recovery blocker from the Consolidated Review on head 2078f19a939f.

• Setup now reaches an explicit recovery state when a full load cannot hydrate after the settled 404/local-copy case, with Retry and Back to projects controls.
• The recovery path keeps the local editor copy and avoids synthesizing blank inputs or issuing a full replacement PATCH. The state is limited to Setup.

Verification: focused Setup and ConfigService suites passed with 90 tests; lint and spec typecheck passed. Recovery assertions failed before the fix and passed afterward, covering pre-load/pending states, the double-404 race, normal hydration, local creation, and retry. Chrome at 1280x1169 and 100% zoom checked the failure, Retry, and keyboard-navigation home states; the race remains covered by HTTP tests rather than a live cloud browser check.

This branch includes the updated #199 parent. Because application workflows target PRs based on main, own-branch application CI is not triggered while stacked; after #199 squash-merges, this PR needs rebasing and retargeting to main for fresh application CI.

Parent #199 CI passed at d301198990da, including Python 3.11/3.12/3.13, UI build/lint/tests and deploy/security checks. This PR's current GitHub check is CLA only; application CI has not run on the child branch. The local child checks are listed above; fresh application CI remains required after rebase and retargeting to main.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [gJustinward]
• gps-readability-typescript-approvers -
  Can grant: [victor-paunescu]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-typescript-approvers -
  Can grant: [victor-paunescu]

[victor-paunescu]

When leaving an editor route (/:id/storyboard, /:id/composition, or /:id/output-video where this.projectView() is 'editor') or leaving a failed Setup load via Back to projects ('/'), resetProjectConfig() and setNewProject() do not reset this.projectView to 'full' or clear this.projectLoadErrorValue.

Consequently, when creating a new project after visiting an editor route (loadProjectConfig('project-a', 'editor') -> resetProjectConfig() -> setNewProject('project-b') -> saveNow() -> loadProjectConfig('project-b', 'full')):

1. this.projectView() remains 'editor', so this.setupInputsLoaded() is false immediately after setNewProject('project-b').
2. When loadProjectConfig('project-b', 'full') runs on navigation to /:id/setup, the short-circuit guard view === this.projectView() || (view === 'editor' && alreadyFull) at line 1587 evaluates to false because this.projectView() is still 'editor'.
3. loadProjectConfig then sets this.projectId.set('project-b'), which causes the projectConfig loader to reset this.setupInputsHydrated.set(false) and dispatch a GET /api/projects/project-b that races the in-flight POST /api/projects (and triggers setupInputsError() === true if the GET returns 404 before the POST commits).

Resetting projectLoadErrorValue, projectId, and projectView in resetProjectConfig() and setNewProject() ensures locally created projects remain hydrated and short-circuit loadProjectConfig regardless of the previous route's view mode.

Suggested change

	this.projectLoadErrorValue.set(undefined);
	this.setupInputsHydrated.set(false);
	this.projectId.set(null);
	this.projectView.set('full');
	this.projectConfig.set({...this.DEFAULT_PROJECT_CONFIG()});
	this.shouldSave = false;
	}
	updateProjectConfig(partial: Partial<ProjectConfig>) {
	this.shouldSave = true;
	this.projectConfig.update(config => {
	return {
	...config,
	...partial,
	};
	});
	}
	setNewProject(uuid: string) {
	// Not persisted yet: the first autosave POSTs /api/projects, where the
	// server stamps createdBy from the verified identity. Left undefined here.
	this.persistedProjectIds.delete(uuid);
	this.projectLoadErrorValue.set(undefined);
	this.setupInputsHydrated.set(true);
	this.projectId.set(null);
	this.projectView.set('full');

[christophervoelpel]

Good catch, I reproduced it. After an editor route (or a failed load), projectView stayed 'editor' and the stale projectLoadErrorValue stayed set. So after "New project", setupInputsLoaded() was false, and Setup was stuck until a reload.

Fix (config.ts):

• resetProjectConfig() now also clears projectLoadErrorValue and resets projectView to 'full'.
• setNewProject() now also clears projectLoadErrorValue and resets projectView to 'full'. Both happen before projectConfig.set(project), so the resource can't reload and overwrite the local project.
• I deliberately did not null projectId inside setNewProject(). A first attempt did, and it broke the existing does not mark a recreated project persisted from a stale load success test, which relies on that contract. resetProjectConfig() already nulls it on the normal "leave project" path.

Test: keeps a locally created project ready after leaving an editor route or failed load in config-mediated.spec.ts. It covers: editor route, then a stale load error, then resetProjectConfig(), then setNewProject(). It then asserts that setupInputsLoaded() is true and stays true after loadProjectConfig(id, 'full'), with no GET /api/projects/<new>, no setupInputsError, and no projectLoadError.

Mutation check: I reverted only the config.ts hunk and this test fails (1 failed / 90 passed); with the fix restored it passes.

Full gate: compile, lint, typecheck:spec, the full UI test suite, and the Python suite are all green (details in the commit).

[victor-paunescu]

In the double-404 recovery test (projectLoadError() === false with !setupInputsHydrated()), we verify that Back to projects is rendered, but we don't currently exercise clicking Retry (config.reloadProjectConfig()) from this state to confirm that a subsequent 200 OK response hydrates inputConfig, preserves the unsaved local editor edit (name: 'In-Flight'), clears setupInputsError(), and opens .setup-container.

Suggested change

	const home = fixture.nativeElement.querySelector('a[href="/"]');
	expect(home?.textContent).toContain('Back to projects');
	expect(fixture.nativeElement.querySelector('.setup-container')).toBeNull();
	expect(config.projectConfig.value().inputConfig).toBeUndefined();
	http.expectNone('/api/projects/proj-unsettled');
	});
	const home = fixture.nativeElement.querySelector('a[href="/"]');
	expect(home?.textContent).toContain('Back to projects');
	expect(fixture.nativeElement.querySelector('.setup-container')).toBeNull();
	expect(config.projectConfig.value().inputConfig).toBeUndefined();
	http.expectNone('/api/projects/proj-unsettled');
	const retry = fixture.nativeElement.querySelector(
	'.loading-state button',
	) as HTMLButtonElement | null;
	expect(retry?.textContent).toContain('Retry');
	retry?.click();
	TestBed.tick();
	const retryGet = http.expectOne('/api/projects/proj-unsettled');
	retryGet.flush({
	id: 'proj-unsettled',
	name: 'Server Name',
	aspectRatio: '16:9',
	resolution: '720p',
	candidateDurationSeconds: 4,
	generateAudio: false,
	numberOfCandidates: 1,
	model: 'veo-default',
	inputConfig: {products: [], composition: 'Recovered composition'},
	storyboard: [],
	audioTracks: [],
	visualOverlays: [],
	});
	TestBed.tick();
	await fixture.whenStable();
	fixture.detectChanges();
	expect(config.setupInputsError()).toBe(false);
	expect(config.setupInputsLoaded()).toBe(true);
	expect(config.projectConfig.value().name).toBe('In-Flight');
	expect(config.projectConfig.value().inputConfig).toEqual({
	products: [],
	composition: 'Recovered composition',
	});
	expect(
	fixture.nativeElement.querySelector('.setup-container'),
	).not.toBeNull();
	});

[christophervoelpel]

Agreed, and applied as suggested (unchanged). The double-404 recovery test now also clicks Retry and flushes a 200. It asserts:

• setupInputsError() is false;
• setupInputsLoaded() is true;
• the unsaved local edit name: 'In-Flight' is kept;
• inputConfig is hydrated to Recovered composition;
• .setup-container renders.

Mutation check: I turned reloadProjectConfig() into a no-op. This test fails, along with 2 other retry tests; with the method restored they all pass.

[gps-readability-bot]

Readability approvals granted:

• TypeScript-Readability-Review - SATISFIED
  [victor-paunescu]