Repository navigation
[3324 by Claude Opus 5.5] Fresh native Plan generation completes on server but studio remains on Define - #3325
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ine → Generating → Review without a reload or a second generation. I haven't tried the fix in the installed native app, and the independent Astra review and real native/mobile check you asked for still need to happen before merge. 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.
|
/review gpt-6-astra Review exact head86c37b780c05f98af077549bcfafc96055fd8699 for #3324. Root observed the real installed native fresh-plan race: delayed generate acknowledgement after autosave/read, server reaches review but the same native Define view remains. Inspect complete fix, stale-fetch/run identity fencing, actual new-draft callback propagation and router-state/back-forward behavior; verify tests reproduce the delayed-ack ordering, not an instant-response fixture that passes on broken code. Check no second generation, unrelated-draft overwrite, indefinite polling or lost prompt. Root actual native/mobile and exact-head checks remain separate gates. Do not merge or publish. A separate logger/dependency fix is still in PR3321 and may later require base integration; this request reviews the current bounded planner fix only. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe fix addresses the fresh-plan acknowledgement race, but needs a localized correction before merge: a delayed acknowledgement can now activate its run on an unrelated draft. ✅ Callback propagation — Fresh-draft generation now passes the acknowledged run and snapshot through SetupWizard to the page’s generation handler. ✅ Realistic race coverage — The integration tests defer generation acknowledgement and exercise stale reads both before and after it, using BrowserRouter and HashRouter. ✅ History recovery — Generating router snapshots receive a background refresh, with tests covering back/forward reopening of completed plans. This was a static review; no commands or tests were run. The original issue excerpt is incomplete, so assessment follows its visible objective and the explicit review request. Actual native/mobile verification remains outstanding. The supplied checks show six pending and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Reject acknowledgements belonging to another draft
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Make browser ordering deterministicThe Playwright fixture releases acknowledgement after fulfilling the first read, with a five-second escape hatch. That does not explicitly establish that React applied the read before acknowledgement. An explicit barrier and assertion would strengthen this browser test. This is optional because the integration tests already control both resolution orders directly. ScoreThe targeted fix and regression coverage are substantial, but the introduced cross-draft activation regression prevents merge readiness. Installed native/mobile validation also remains unverified. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F1 S1 |
|
✅ Applied the requested follow-up changes in commit c5c51b3
Summary of ChangesI 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), F1 fix
Tests
S1,
No preview images: nothing visible changed. Possible separate issue (not changed, not tested): in 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…e 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.
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/<id>', { 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
|
/review gpt-6-astra Review exact head c5c51b3 after F1/S1 remediation. Verify originating-draft identity is checked before any activation or polling mutation, including snapshot-less callbacks, both pending-fetch and already-loaded destination drafts, and that fresh autosave/generation ordering still works for BrowserRouter and native HashRouter. Inspect the explicit rendered-read barrier and original generation regression coverage. Root exact-source tests and rendered validation are separate ongoing gates. The agent notes a pre-existing fresh-plan navigation-after-leave possibility; distinguish it from introduced regressions and assess whether this fix makes it worse. Do not merge or publish. Delivery retry on this same review request: the original revision had no hosted task, pending-comment entry or matching service-log receipt; a later review request on PR3327 was accepted normally. Please review this exact head once. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe supplied diff for head ✅ Identity checked before activation — ✅ Acknowledgement reaches the displayed draft — Fresh creation propagates the acknowledged run through ✅ Regression ordering is explicit — The page tests cover reads resolving before and after acknowledgement under BrowserRouter and HashRouter, plus autosave-first generation, history navigation, and prompt preservation on failure. The rendered test holds acknowledgement until the server-assigned name appears in the document title, establishing that the pre-generation read was applied. This was a static review of the supplied code; no commands or tests were run. The supplied status reports no failed checks, with four pending. The original generation regression coverage remains intact. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Complete snapshot-less transition coverageExtend the A-to-B acknowledgement test to invoke A’s captured callback without a snapshot both while B’s fetch is pending and after B loads. The existing tests separately exercise snapshot-less rejection and cross-draft transitions, and the unconditional identity guard handles their combination correctly. Combining them would strengthen regression coverage without requiring a production change. S3: 🟢 Fence navigation after leavingConsider a separate follow-up for ScoreThe fix is focused, its identity checks address the reported remediation, and the added tests model the important delayed-acknowledgement ordering. No code blocker was established; independent execution and rendered validation remain outstanding gates. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
CI failed: Full Test Suite Shard 2/4 Please investigate and fix this CI failure.
Failure evidence
|
|
✅ Applied the requested follow-up changes in commit af6936a Summary of ChangesI 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 The jobs fix f5e3510, which this branch includes, made Fix. I added the two missing functions to the stand-in at No product code changed and nothing visible changed, so there are no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
|
CI failed: Run Full Test Suite Please investigate and fix this CI failure.
Failure evidence
|
Full Test Suite ResultsValidation failed during: Test shard (shard 2).
View shard 2/4 outputDetails truncated; see the uploaded artifacts for complete failure rows and logs. |
|
/merge Integrate current main including merged PR #3321 (960a437). That reviewed change contains the logger containment fix, ip-address 10.7.4 lockfile resolution and the missing delayPRCommentJobInPlace/predecessorWaitDelay recovery-fixture mock exports. PR3325 CI at c5c51b3 demonstrably failed importing the first missing export from that old fixture; sanitized shard2 output retained independently. Preserve this PR's reviewed UI changes and assertions; do not create another workaround for a fixture already fixed on main. ProPR owns Git finalization. Do not merge this PR into main, publish or deploy. Root will inspect the exact resulting head, refresh Astra review and verify current checks separately. |
…ally (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 f5e3510, 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 (960a437). 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
|
ℹ️ Analyzed the follow-up request by @propr-dev[bot], @github-actions[bot] Analysis SummaryNo new changes were needed. The fix for this failure is already on the branch, in the latest commit
The branch still does not contain main or #3321 ( Nothing visible changed, so there are no previews. No code changes were necessary based on the current state of the branch. 🤖 Analysis Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
|
🔀 Auto-merged No conflicts were found — the merge was verified by an AI agent. 🤖 Verification Details
System-triggered merge conflict resolution |
…ude-opus-5-5-fresh-native-plan-generat-20261022-2234-oc0
|
/fix S3 Root independently reproduced the previously optional navigation concern with the exact c5c51b3 renderer and real App routing. This is now a confirmed UI finding during the requested full Linux/mobile validation, so please fix it in this planner PR. Reproduction (API boundary stubbed, no provider call): open /plans; navigate to /studio/new; type a prompt and click Generate exactly once before autosave. POST /drafts returns navigation-owned-draft, and the ordinary in-place callback replaces the route with /studio/navigation-owned-draft while POST /generate remains pending. Use the browser Back action and verify /plans is shown. Resolve the generation acknowledgement with runId owned-run/status generating. Observed URL becomes /studio/navigation-owned-draft again. Exactly one generation occurred. The assertion that /plans remains selected fails. Root retained the independent test, JSON observation, screenshot and trace at /tmp/propr-pr3325-c5c51b3-independent/navigation-diagnostic on the Linux validation machine; reproduce from the steps here rather than relying on access to that local path. Required behavior: after the user deliberately leaves a pending fresh-plan flow, its acknowledgement must not navigate back, replace the user's current history entry, activate another draft or overwrite that draft's state. Also cover navigation to a different existing draft while the acknowledgement is pending. Preserve the backend-created draft/run so it can be reopened normally; do not cancel generation merely because the user navigated away, and do not generate again. Preserve all existing original-race/F1/S1 assertions: intentional /studio/new -> saved draft ID in-place navigation must still deliver the acknowledged run and reach Generating then Review automatically with one create/one generation. Cover BrowserRouter and native HashRouter, browser Back, switching drafts and returning to the generated draft. Please ensure any ownership fence distinguishes intentional autosave replacement from actual departure. Keep permission, credential, provider and repository settings unchanged. Do not merge, publish or deploy. |
|
✅ Applied the requested follow-up changes in commit 65e4b86
Summary of ChangesI 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 The fix.
Tests
Results
HashRouter is covered by the jsdom page tests only, because the Playwright web build uses BrowserRouter. The 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. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ation 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.
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/<id>', { 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/<id>` 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/<id>`. 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
|
/review gpt-6-astra Review exact head65e4b864fc2594330f8388b80359eddc74ab9105, especially S3 history-entry ownership after the independently reproduced Back/late-ack navigation takeover. Inspect the complete PR and preservation of original fresh-generation/F1/S1 behavior, BrowserRouter and actual native HashRouter path semantics, history keys/index, lazy route transitions, delayed creation callback and unmount safety. Check switching to another draft and Back/Forward, including returning to the originating draft before an acknowledgement arrives. Ensure no unintended navigation, overwritten unrelated draft, cancelled server run, duplicate creation/generation, or suppression of normal intentional autosave replacement. Distinguish introduced defects from pre-existing concerns. Root independent failing-before/passing-after diagnostic, exact-source unit/browser tests and real Android Goal checks are separate ongoing gates. Do not merge, publish or deploy. Retry after the prior review terminated with token-relay timeout at6354359281; it supplied no code review. Review this same exact head once. No permissions, credentials or relay configuration may be changed. |
|
Posted 0 review, 1 failed: |
🔍 AI Code Review — gpt-6-astra❌ Review failed: GitHub token relay unreachable at https://webhook.propr.dev/v1: The operation was aborted due to timeout |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR repairs fresh-generation activation and adds history-entry checks against late navigation takeover. It needs a localized correction before merge: the new background resynchronization can overwrite another selected draft. ✅ Acknowledgement fencing — Activation checks the originating draft ID before changing run identity, including snapshot-less callbacks. ✅ Navigation ownership — Fresh-generation acknowledgements check mounted state and browser history key, index, and runtime-aware pathname before navigating. ✅ Delayed-response coverage — Tests exercise stale reads both before and after acknowledgement, plus late creation and Back navigation. This is a static review of the supplied diff for Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F2: 🔴 Fence background reads when switching drafts
SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Cover departure and return explicitlyAdd coverage for leaving and returning to the originating history entry before acknowledgement, including a suspended lazy-route transition. The current tests cover acknowledgement while away and reopening afterward. This additional ordering would clarify whether ownership is intentionally recoverable when the same entry remains mounted; no separate defect in that ordering was established. ScoreThe acknowledgement and navigation fixes are focused and well covered, but the newly introduced resynchronization needs protection against overwriting another draft. Pending checks alone do not reduce the score. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F2 Address the exact-head65e4b86 Astra blocker6354437178. Root67 targeted units, build and typecheck pass, but they do not cover the stale background resync identified by F2 and do not override that finding. Preserve selection ownership across every fetch completion: a late success, error or loading-finalization from draft A must not alter draft B or its active run. Cover a generating history snapshot whose background GET stays pending, navigation to B, B loading/completing, then A resolving. Also exercise failure and re-selection A -> B -> A so an obsolete read cannot replace a newer selection/result merely because the ID matches again. Keep existing activation-sequence protection against pre-ack reads, original fresh-generation cases, F1 draft/run fencing and S3 history-navigation ownership intact. Avoid cancelling or repeating real generation requests, permission/credential/configuration changes, and unrelated refactors. Root independent diagnostic and eventual signed-native validation remain separate. Do not merge, publish or deploy. |
|
✅ Applied the requested follow-up changes in commit b663944
Summary of ChangesI 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 (
One behaviour change for the same draft: if two reads overlap, only the newest result is used. Before, whichever finished last won. New tests (
Verification:
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. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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. 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
|
/review gpt-6-astra Review exact headb663944f7f162c29c2139450a2d27f869896d9cd after F2 remediation, and the complete planner PR for preservation of F1/S1/S3. Check selected-draft ownership, read sequence invalidation on draft change/unmount, stale callbacks, A-B-A reselection, same-draft overlapping reads, loading/error and activeRunId protection, and interaction with generation acknowledgement and socket updates. Root independently reproduced F2 at65e4b86 with three failing real-hook/mock-boundary tests (late success/error/ABA); those exact assertions will run against this head. S3 independently failed before and passed at65e4b86 in real Chromium. Exact-source tests, currentCI and eventual signed-native acceptance remain separate gates. Do not merge, publish or deploy. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe supplied diff addresses the fresh-generation race and the previously reported cross-draft background-read overwrite. No verified merge blocker remains in the changed code. The PR is ready within the reviewed code scope, conditional on pending checks and the separately planned acceptance gates. ✅ Read completion ownership — Fetch sequencing and selected-draft checks fence success, failure, and loading completion across draft changes, including A → B → A reselection. Newer same-draft reads supersede older reads. ✅ Acknowledgement fencing — Activation checks the originating draft and snapshot identity before changing run state. Snapshot-backed activation invalidates pre-acknowledgement reads. ✅ Navigation protection — Fresh creation and generation callbacks check browser history ownership, including native hash path semantics. The browser regression includes an explicit rendered-read barrier before acknowledgement. This was a static review of the supplied diff identified as Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S5: 🟢 Cover remaining read interleavingsThe new selection tests directly cover late success, late error, A → B → A, and stale callbacks while another draft is selected. Explicit tests for overlapping reads without a selection change and completion after unmount would strengthen coverage of the new sequence mechanism. Include a newer background read superseding a loading read, checking loading, error, and run identity. Static inspection supports these paths; this is optional regression coverage. S6: 🟢 Fence existing-generation failure callbacksThe existing-generation handler checks draft identity after successful ScoreThe changed implementation closes the demonstrated selection race while preserving the fresh-generation and navigation protections. Coverage is substantial; pending CI and signed-native acceptance remain separate validation gates. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3324
Branch:
3324/claude-opus-5-5-fresh-native-plan-generat-20261022-2234-oc0Commits: ✅ Changes committed (86c37b7)
AI Processing Completed
Execution Details:
Summary:
I reproduced the bug in tests and fixed it. A fresh plan now goes Define → Generating → Review without a reload or a second generation. I haven't tried the fix in the installed native app, and the independent Astra review and real native/mobile check you asked for still need to happen before merge.
Cause. The bug shows up when Generate is clicked before the autosave delay finishes:
GETof that draft.POST /generate, because it prepares the repository before marking the draftgenerating. The page'sGETcomes back first, withstatus: draft./generateis acknowledged,useDraftCreationnavigates to/studio/:idand puts the generating draft in the router state. But inApp.tsxthe two studio routes render the same component tree, soPlanStudioPageis not remounted. ItsuseDraftstill holds thedraftcopy and ignores the router state.My first Playwright mock answered
/generateinstantly, 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/generateis 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:SetupWizard.tsx,setupWizardHooks.ts,PlanStudioPage.tsx: pass the draft along with the run ID.Tests:
PlanStudioPage.freshGenerate.test.tsx(new): uses the real page, routing,useDraftandSetupWizard, 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.setupWizardHooks.test.tsxwas updated for the extra argument.tscandeslint --max-warnings 0are clean.Preview screenshots of the Generating and Review states at 390px and in the native minimum window are attached.
Detailed Logs:
16498ad7-5cfe-4649-bf32-fc5e09f65f58Log files stored at:
/usr/src/app/logs/executions/issue-3324-2026-10-22T23-04-38-139Z-conversation.json/usr/src/app/logs/executions/issue-3324-2026-10-22T23-04-38-139Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3324.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
Native shell minimum window: Generating
Same fresh-plan flow at the installed app's 880x620 minimum window with the Linux titlebar
Original unavailable: managed storage is unavailable or the upload has expired.
Fresh plan: Generating (390px)
Generate clicked on /studio/new before the autosave debounce; the studio now switches to the acknowledged run's Generating view instead of staying on Define
Original unavailable: managed storage is unavailable or the upload has expired.
Native shell minimum window: Review
Completed plan shown in Review in the native shell without reload or a second generation
Original unavailable: managed storage is unavailable or the upload has expired.
Fresh plan: Review (390px)
Server completion of the same run automatically shows the generated plan in Review
Original unavailable: managed storage is unavailable or the upload has expired.