Repository navigation
[3270 by Claude Opus 5.5] Plan Generate races autosave and leaves a duplicate empty draft - #3273
Merged
integry merged 1 commit intoOct 19, 2026
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…eation as autosave and preview, so one click persists exactly one draft. The full propr-ui unit suite passes (324 files, 2959 tests), and typecheck and lint are clean. Cause: an immediate Generate created draft A directly and then waited on `POST /generate`. The 1s autosave debounce was still pending, fired during that wait, and created draft B. In the issue's request log, B's POST comes right after the generate call. Fix: - `useDraftCreation.ts` (new): Generate in new mode now gets its draft from `ensureDraftCreated()`. That call cancels the pending debounce, joins a creation already in flight, or returns the draft already created. It never calls `createDraft` itself. Once it has the draft, it: - saves the current prompt if it changed after an earlier autosave started. - saves the current settings (branch, model, granularity, context, manual/excluded files) only if they differ from what the draft has. - uploads only attachments the autosave didn't already upload. - starts generation, then navigates in place exactly as before. A guard ignores a second Generate while the first is still starting. - `useAutoDraftCreation.ts`: records which local files it uploaded and exposes `uploadPendingLocalFiles`, so no file is uploaded twice or lost. - `SetupWizard.tsx`: wires Generate to the autosave hook's functions. The to-do link and the `onDraftCreated` callback still come from the single creation path. - `setupWizardHooks.ts`: the old direct-create Generate path is removed. Autosave itself is unchanged, the existing-draft Generate path is unchanged, and no existing drafts are touched. One small behaviour change: if draft creation fails, the inline error now just says "Failed to create draft". The specific reason still appears in the autosave error toast. Tests: - `SetupWizard.generateDraftRace.test.tsx` renders the real SetupWizard and hooks, with only the API mocked: - Immediate Generate with an attachment and a to-do: `POST /generate` is held open past the debounce. Result: one draft with the right prompt and to-do, one generation, one upload, all updates on that draft. I checked this against the old code and it fails there with two drafts. - Generate after the autosaved draft is already in place: no second draft. - Generation fails to start, then the user retries: the same draft is generated twice and no new draft is created. - `useDraftCreation.test.tsx` tests the two hooks wired as in SetupWizard: - The pending debounce is cancelled and one draft is created. - Generate joins an autosave already in flight, and keeps the edited prompt, model, manual files and a file added mid-flight. - A failed settings save still lets generation go ahead, with the warning. - A failed creation shows an error and the retry creates one draft. - A double click starts only one generation. - The old `useDraftCreation` tests in `setupWizardHooks.test.tsx` are replaced by these. The change isn't visible on screen (the only difference is one fewer stray Draft entry), so I didn't capture previews.
Owner
|
/review gpt-6-astra |
Contributor
Author
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
Author
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR fixes the visible duplicate-draft race by routing Generate through autosave’s shared creation promise. It appears merge-ready within the reviewed scope, conditional on the six pending checks completing successfully. ✅ Shared draft creation — Generate cancels the pending autosave debounce and reuses an existing or in-flight draft instead of issuing an independent creation request. ✅ Focused regression coverage — Added tests cover delayed generation responses, single-draft persistence, attachment deduplication, and retrying generation on the same draft. This was a static review; no commands or tests were run. The issue description is truncated, so objective validation is limited to the visible requirements. The supplied diff was reviewed as complete. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise the actual native routeConsider extending ScoreThe implementation addresses the reported race with a localized change and targeted regression tests. No merge-blocking defect was verified; the authoritative checks report zero failures, with six still pending. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
propr-dev Bot
pushed a commit
that referenced
this pull request
Oct 19, 2026
…indow, the changed file can now be scrolled into view with a finger or the mouse wheel, tapped, its diff opened and closed. I tested this in headless Chromium against stubbed task data. It has not been run against real API data or on a real phone. Cause. From 640px wide up, the task page uses the desktop layout. It pins the task header and gives only the area below it its own scroll. In a short window that area was too thin: about 100px in my reproduction, 58px in your measurement. The pinned Timeline and Files headings and the log bar covered nearly all of it, so the file row could never be tapped. Fix. - New `useShortWorkspace.ts`: it measures how much height the header leaves for the timeline/files area. It counts the collapsed log bar and the pinned headings, and allows about 184px for rows. It also respects the visible area when an on-screen keyboard shrinks it, but ignores pinch zoom. The measurement doesn't depend on which layout is active, so it can't flip back and forth. - In `TaskDetails/index.tsx`, when there isn't enough room: - the whole task scrolls as one page, the way phones already do. - the header scrolls away with it, but all its metadata and actions are still there when you scroll back up. - the Timeline and Files headings still pin, now to the top of the page. - the execution-log bar stays pinned at the bottom, and opening it keeps its normal height. - extra scroll padding keeps a focused or revealed row clear of the pinned headings and the log bar. - Unchanged: tall desktop windows, portrait phones, how the timeline heading sits above its content, and returning focus to the file row when the diff closes. The diff dialog's existing scroll lock still holds the page in place. Tests. The new `e2e/task-details-short-viewport.pw.ts` covers 844×390 touch landscape, 880×480 desktop, 320×640 and 390×844 portrait, and 1280×820 desktop. Each uses a completed PR task with a long two-line title and subtitle, 8 runs and one modified 121-line Markdown file. It: - scrolls only with real touch swipes or wheel input. - checks that a tap at the row's centre actually reaches the row and that the visible area is usable. - opens and closes the diff, then checks focus and scroll position are restored. - opens and closes the execution log. - scrolls back up and opens the header's "More task actions" menu. On the old code the two short cases fail the way the issue describes: at 844×390 the row can't be scrolled to a tappable spot, and at 880×480 only 51px of the area is visible. One existing test changed behaviour. In `task-timeline-heading-layering.pw.ts`, the case that docks the task beside the list at 1280×820 with 150% text now uses the new page-scroll layout. There, the enlarged header is 423px tall and leaves only about 97px for rows, the same problem as the issue. The test now scrolls the whole page and still checks that nothing paints over the Timeline heading. Results. All 270 tests in the `task-*` browser suites pass, as do 424 TaskDetails unit tests, eslint with zero warnings and tsc. The new spec passed 3 times in a row. I didn't touch any files from #3273, #3274 or #3275, and added no dependencies. The 844×390 and 880×480 previews show the revealed file row and the opened diff. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used. This task didn't need them.
integry
pushed a commit
that referenced
this pull request
Oct 20, 2026
…d files covered and untappable (#3277) * I fixed #3276. At 844×390 touch landscape and in an 880×480 desktop window, the changed file can now be scrolled into view with a finger or the mouse wheel, tapped, its diff opened and closed. I tested this in headless Chromium against stubbed task data. It has not been run against real API data or on a real phone. Cause. From 640px wide up, the task page uses the desktop layout. It pins the task header and gives only the area below it its own scroll. In a short window that area was too thin: about 100px in my reproduction, 58px in your measurement. The pinned Timeline and Files headings and the log bar covered nearly all of it, so the file row could never be tapped. Fix. - New `useShortWorkspace.ts`: it measures how much height the header leaves for the timeline/files area. It counts the collapsed log bar and the pinned headings, and allows about 184px for rows. It also respects the visible area when an on-screen keyboard shrinks it, but ignores pinch zoom. The measurement doesn't depend on which layout is active, so it can't flip back and forth. - In `TaskDetails/index.tsx`, when there isn't enough room: - the whole task scrolls as one page, the way phones already do. - the header scrolls away with it, but all its metadata and actions are still there when you scroll back up. - the Timeline and Files headings still pin, now to the top of the page. - the execution-log bar stays pinned at the bottom, and opening it keeps its normal height. - extra scroll padding keeps a focused or revealed row clear of the pinned headings and the log bar. - Unchanged: tall desktop windows, portrait phones, how the timeline heading sits above its content, and returning focus to the file row when the diff closes. The diff dialog's existing scroll lock still holds the page in place. Tests. The new `e2e/task-details-short-viewport.pw.ts` covers 844×390 touch landscape, 880×480 desktop, 320×640 and 390×844 portrait, and 1280×820 desktop. Each uses a completed PR task with a long two-line title and subtitle, 8 runs and one modified 121-line Markdown file. It: - scrolls only with real touch swipes or wheel input. - checks that a tap at the row's centre actually reaches the row and that the visible area is usable. - opens and closes the diff, then checks focus and scroll position are restored. - opens and closes the execution log. - scrolls back up and opens the header's "More task actions" menu. On the old code the two short cases fail the way the issue describes: at 844×390 the row can't be scrolled to a tappable spot, and at 880×480 only 51px of the area is visible. One existing test changed behaviour. In `task-timeline-heading-layering.pw.ts`, the case that docks the task beside the list at 1280×820 with 150% text now uses the new page-scroll layout. There, the enlarged header is 423px tall and leaves only about 97px for rows, the same problem as the issue. The test now scrolls the whole page and still checks that nothing paints over the Timeline heading. Results. All 270 tests in the `task-*` browser suites pass, as do 424 TaskDetails unit tests, eslint with zero warnings and tsc. The new spec passed 3 times in a row. I didn't touch any files from #3273, #3274 or #3275, and added no dependencies. The 844×390 and 880×480 previews show the revealed file row and the opened diff. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used. This task didn't need them. * feat(ai): Your exact-head failures didn't reproduce in this container, but while ruling out a product cause I found and fixed a real restoration defect in the short layout. That defect plausibly explains the 6px landscape failure. The 1280×820 failure is still unconfirmed: my best explanation is that the wheel scroll hadn't finished when the baseline was measured, and the new wait now covers that. Your exact-head failures didn't reproduce in this container, but while ruling out a product cause I found and fixed a real restoration defect in the short layout. That defect plausibly explains the 6px landscape failure. The 1280×820 failure is still unconfirmed: my best explanation is that the wheel scroll hadn't finished when the baseline was measured, and the new wait now covers that. **Product defect.** In the short layout, the bottom scroll padding was `scroll-pb-12` (48px), but the pinned log bar is 37px (36px plus a 1px top border). When a file row sat fully visible within about 12px above the bar, putting focus back on it as the diff closed counted it as covered and scrolled the page. A diagnostic run placed the row at fixed distances from each edge, then opened and closed the diff: | Row's gap above log bar | 0px | 2px | 5px | 10px | 20px+ | |---|---|---|---|---|---| | Scroll movement, 844×390 and 880×480 | 11px | 9px | 6px | 1px | 0 | The top edge and every position at 1280×820 moved 0px. I set the padding in `useShortWorkspace.ts` to exactly the bar's height (`scroll-pb-[calc(2.25rem+1px)]`). After that change, every position moved 0px in all three sizes. The 1280×820 layout has no such padding, so its 4px isn't explained by this defect. **Test changes** (`e2e/task-details-short-viewport.pw.ts`): - **Waiting for the scroll to end:** the old check was three animation frames without movement. It now waits for the scroll's `scrollend` event and then 300ms with nothing moving. If `scrollend` never fires, it falls back to 5s. - **Baseline position:** it is now measured where the row rests just before it is tapped: after the swipe's momentum, after the preview capture, with a fresh tap point. - **New check in the 880×480 case:** the mouse wheel moves the row to 1–6px above the log bar, then the diff is opened and closed. With the old padding this fails with an 8px difference; with the fix it passes. - **Timing:** I removed a duplicate wait inside the scroll-into-view step and gave these five cases a 60s timeout. Each case does many swipes and each now waits for its momentum to end, so landscape takes about 14s on an idle host and 30–32s under full CPU load, which is over the default 30s. - The 1px limit, the focus and tap-target checks, and real touch/wheel input are unchanged. There are no forced clicks, and the test never sets the scroll position itself. **Results:** - **Repeat runs:** the five viewport cases plus the nine timeline-heading cases passed three times with preview capture and three times without, 84/84. - **Under full CPU load:** the five viewport cases passed once with capture and once without. - **Unit tests, lint and type check:** all 424 TaskDetails unit tests pass, and eslint (zero warnings) and tsc are clean. - **One failure along the way:** before the 60s timeout, an overloaded run timed out in the landscape case and in the unchanged timeline-heading enlarged-text case. **Limits:** - **Old wait never failed here.** It never returned before the scroll ended in this container: not with the CPU slowed, not under load, not with smooth-scrolling Chromium. The 1280×820 explanation therefore comes from your 24-frame result, not from a failure I reproduced. - **Edge diagnostic was not real input.** It set the scroll position directly, and it is not committed. - **Remaining gates:** a real phone, real API data, and signed-native integration are still your release gates. There are no visual previews because the change isn't visible in a still image. Separately, Gmail, Google Calendar and Google Drive need authorizing in your claude.ai connector settings before they can be used; this task didn't need them. PR: #3277 Comment by: @integry (ID: 6279336708) Model: Claude Opus 5.5 * feat(ai): I fixed the Validate Changes failure. The cause was a race in this PR, not the logs dialog: the listener-leak check in `e2e/dialog-keyboard-viewport.pw.ts` measured its baseline before the task page had finished loading. I fixed the Validate Changes failure. The cause was a race in this PR, not the logs dialog: the listener-leak check in `e2e/dialog-keyboard-viewport.pw.ts` measured its baseline before the task page had finished loading. **What went wrong.** The new `useShortWorkspace` hook adds a resize listener on the browser's visual viewport once the task header mounts. The test records the starting (`idle`) listener count right after `page.goto`, before the task data has loaded. So that count is 0, and after the dialog closes the count is 1, the hook's listener for the page itself. The dialogs weren't leaking anything. Locally, this failed every time in the Follow Up test and in both Logs tests (fonts loaded and blocked), not only the one CI reported. **Fix (tests only, no product code changed):** - `e2e/task-logs.fixture.ts`: a new `taskShown(page)` helper waits until the task page has rendered. It reuses the "more actions" button lookup that `openTaskAction` already uses. - `e2e/dialog-keyboard-viewport.pw.ts`: the Follow Up and Logs tests now call `taskShown(page)` before measuring `idle`. The baseline therefore includes the task page's own listeners. The check still fails if a dialog leaves a listener behind. **Verification:** - `dialog-keyboard-viewport`, `task-logs-*` and `task-details-short-viewport` pass, each run twice with retries off (46 passed, 0 failed). Before the fix, `dialog-keyboard-viewport` failed 9 of 15 runs. - ESLint (`--max-warnings 0`) and `tsc --noEmit` pass on the changed files. This change isn't visible in the UI, so there are no previews. PR: #3277 Comment by: @propr-dev[bot] (ID: 6280778125) Comment by: @github-actions[bot] (ID: 6280778384) Model: Claude Opus 5.5 --------- Co-authored-by: propr-dev[bot] <1316198+propr-dev[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Implementation Summary
Closes #3270
Branch:
3270/claude-opus-5-5-plan-generate-races-autos-20261019-2145-fh2Commits: ✅ Changes committed (13434ec)
AI Processing Completed
Execution Details:
Summary:
I've fixed the bug: Generate now uses the same single-flight draft creation as autosave and preview, so one click persists exactly one draft. The full propr-ui unit suite passes (324 files, 2959 tests), and typecheck and lint are clean.
Cause: an immediate Generate created draft A directly and then waited on
POST /generate. The 1s autosave debounce was still pending, fired during that wait, and created draft B. In the issue's request log, B's POST comes right after the generate call.Fix:
useDraftCreation.ts(new): Generate in new mode now gets its draft fromensureDraftCreated(). That call cancels the pending debounce, joins a creation already in flight, or returns the draft already created. It never callscreateDraftitself. Once it has the draft, it:A guard ignores a second Generate while the first is still starting.
useAutoDraftCreation.ts: records which local files it uploaded and exposesuploadPendingLocalFiles, so no file is uploaded twice or lost.SetupWizard.tsx: wires Generate to the autosave hook's functions. The to-do link and theonDraftCreatedcallback still come from the single creation path.setupWizardHooks.ts: the old direct-create Generate path is removed.Autosave itself is unchanged, the existing-draft Generate path is unchanged, and no existing drafts are touched.
One small behaviour change: if draft creation fails, the inline error now just says "Failed to create draft". The specific reason still appears in the autosave error toast.
Tests:
SetupWizard.generateDraftRace.test.tsxrenders the real SetupWizard and hooks, with only the API mocked:POST /generateis held open past the debounce. Result: one draft with the right prompt and to-do, one generation, one upload, all updates on that draft. I checked this against the old code and it fails there with two drafts.useDraftCreation.test.tsxtests the two hooks wired as in SetupWizard:useDraftCreationtests insetupWizardHooks.test.tsxare replaced by these.The change isn't visible on screen (the only difference is one fewer stray Draft entry), so I didn't capture previews.
Detailed Logs:
b93dec8b-f516-4de7-9b25-921ee34f7d66Log files stored at:
/usr/src/app/logs/executions/issue-3270-2026-10-19T21-58-34-469Z-conversation.json/usr/src/app/logs/executions/issue-3270-2026-10-19T21-58-34-469Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3270.
💡 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!