fix(sessions): add cancel control to cloud initializing screen#2621
fix(sessions): add cancel control to cloud initializing screen#2621posthog[bot] wants to merge 1 commit into
Conversation
A provisioning cloud task renders CloudInitializingView as a full-screen spinner with no interactive controls. If the SSE stream never delivers a status transition, the task is stuck on "Getting things ready…" with no way to stop, cancel, or resume. Surface a Cancel button during the initializing render path, wired to the existing onCancelPrompt callback (cancelPrompt -> cancelCloudPrompt), which already sends a `cancel` command over tRPC for null/queued/in_progress runs and reuses the TASK_RUN_CANCELLED analytics. Generated-By: PostHog Code Task-Id: 677a6fa9-0f43-4d72-aa8a-12dd3a008619
|
React Doctor could not complete this scan.
Reviewed by React Doctor for commit |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/ui/src/features/sessions/components/CloudInitializingView.tsx:51-55
The `if (cancelling) return` guard is redundant — React does not fire `onClick` on a `disabled` button, so the check can never be reached once `cancelling` is `true`. The "Cancelling…" label and `disabled` attribute on the button are already the single point of protection. This violates the "no superfluous parts" simplicity rule, and the duplicate guard also means the same invariant is expressed in two places (once in the handler, once in the JSX).
```suggestion
const handleCancel = () => {
setCancelling(true);
onCancel?.();
};
```
### Issue 2 of 3
packages/ui/src/features/sessions/components/CloudInitializingView.tsx:44-55
`cancelling` is never reset to `false`. If `onCancel` is invoked but the underlying request fails (and the component stays mounted in the initialising state), the button is permanently locked in "Cancelling…" with no retry path. Because `onCancel` is typed as `() => void`, the caller can't surface a failure back here. Consider widening the signature to `onCancel?: () => Promise<void>` and resetting `cancelling` in a `catch`, or at minimum resetting it in `finally`, so a failed cancel attempt doesn't permanently disable the control.
### Issue 3 of 3
packages/ui/src/features/sessions/components/CloudInitializingView.test.tsx:23-46
The first two tests cover the same boolean condition (`onCancel` provided vs omitted) and their assertions mirror each other. Per the team's parameterised-test preference these could be collapsed into a single `it.each`, e.g. `[[null, true], [undefined, false]]` — keeping the shared setup in one place and making it easy to add more `cloudStatus`/`onCancel` combinations later.
Reviews (1): Last reviewed commit: "fix(sessions): add cancel control to clo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const handleCancel = () => { | ||
| if (cancelling) return; | ||
| setCancelling(true); | ||
| onCancel?.(); | ||
| }; |
There was a problem hiding this comment.
if (cancelling) return guard is redundant — React does not fire onClick on a disabled button, so the check can never be reached once cancelling is true. The "Cancelling…" label and disabled attribute on the button are already the single point of protection. This violates the "no superfluous parts" simplicity rule, and the duplicate guard also means the same invariant is expressed in two places (once in the handler, once in the JSX).
| const handleCancel = () => { | |
| if (cancelling) return; | |
| setCancelling(true); | |
| onCancel?.(); | |
| }; | |
| const handleCancel = () => { | |
| setCancelling(true); | |
| onCancel?.(); | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/sessions/components/CloudInitializingView.tsx
Line: 51-55
Comment:
The `if (cancelling) return` guard is redundant — React does not fire `onClick` on a `disabled` button, so the check can never be reached once `cancelling` is `true`. The "Cancelling…" label and `disabled` attribute on the button are already the single point of protection. This violates the "no superfluous parts" simplicity rule, and the duplicate guard also means the same invariant is expressed in two places (once in the handler, once in the JSX).
```suggestion
const handleCancel = () => {
setCancelling(true);
onCancel?.();
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const [revealed, setRevealed] = useState(false); | ||
| const [cancelling, setCancelling] = useState(false); | ||
| useEffect(() => { | ||
| const timer = setTimeout(() => setRevealed(true), REVEAL_DELAY_MS); | ||
| return () => clearTimeout(timer); | ||
| }, []); | ||
|
|
||
| const handleCancel = () => { | ||
| if (cancelling) return; | ||
| setCancelling(true); | ||
| onCancel?.(); | ||
| }; |
There was a problem hiding this comment.
cancelling is never reset to false. If onCancel is invoked but the underlying request fails (and the component stays mounted in the initialising state), the button is permanently locked in "Cancelling…" with no retry path. Because onCancel is typed as () => void, the caller can't surface a failure back here. Consider widening the signature to onCancel?: () => Promise<void> and resetting cancelling in a catch, or at minimum resetting it in finally, so a failed cancel attempt doesn't permanently disable the control.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/sessions/components/CloudInitializingView.tsx
Line: 44-55
Comment:
`cancelling` is never reset to `false`. If `onCancel` is invoked but the underlying request fails (and the component stays mounted in the initialising state), the button is permanently locked in "Cancelling…" with no retry path. Because `onCancel` is typed as `() => void`, the caller can't surface a failure back here. Consider widening the signature to `onCancel?: () => Promise<void>` and resetting `cancelling` in a `catch`, or at minimum resetting it in `finally`, so a failed cancel attempt doesn't permanently disable the control.
How can I resolve this? If you propose a fix, please make it concise.| it("renders a cancel control while provisioning when onCancel is provided", () => { | ||
| render( | ||
| <Theme> | ||
| <CloudInitializingView cloudStatus={null} onCancel={() => {}} /> | ||
| </Theme>, | ||
| ); | ||
| reveal(); | ||
| expect(screen.getByText("Getting things ready…")).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByRole("button", { name: "Cancel" }), | ||
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("omits the cancel control when no handler is provided", () => { | ||
| render( | ||
| <Theme> | ||
| <CloudInitializingView cloudStatus="queued" /> | ||
| </Theme>, | ||
| ); | ||
| reveal(); | ||
| expect( | ||
| screen.queryByRole("button", { name: "Cancel" }), | ||
| ).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
onCancel provided vs omitted) and their assertions mirror each other. Per the team's parameterised-test preference these could be collapsed into a single it.each, e.g. [[null, true], [undefined, false]] — keeping the shared setup in one place and making it easy to add more cloudStatus/onCancel combinations later.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/sessions/components/CloudInitializingView.test.tsx
Line: 23-46
Comment:
The first two tests cover the same boolean condition (`onCancel` provided vs omitted) and their assertions mirror each other. Per the team's parameterised-test preference these could be collapsed into a single `it.each`, e.g. `[[null, true], [undefined, false]]` — keeping the shared setup in one place and making it easy to add more `cloudStatus`/`onCancel` combinations later.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
Users running cloud tasks could get permanently stuck on the "Getting things ready…" screen with no way to stop, cancel, or resume — forcing them to abandon their work (customer ticket #1581). During provisioning,
SessionViewrendersCloudInitializingViewas its only child: a full-screen spinner with zero interactive controls. The only Stop button lives inPromptInput, which isn't mounted in theisInitializingbranch. If the SSE stream never delivers a status transition (cloudStatusstaysnull/queued), the task displays this message indefinitely.Changes
CloudInitializingViewnow accepts an optionalonCancelhandler and renders a Cancel button (with a "Cancelling…" pending state to prevent double-clicks).SessionViewwires that to the existingonCancelPromptcallback, which routes throughcancelPrompt→cancelCloudPromptand already sends acancelcommand over tRPC for non-terminal (null/queued/in_progress) runs, reusing the existingTASK_RUN_CANCELLEDanalytics.No service behavior changed — the cloud
cancelcommand cancels the current turn (mirroring localcancelPrompt), so the fix is a localized UI affordance rather than an optimistic status transition.How did you test this?
CloudInitializingView.test.tsxcovering: the Cancel control renders while provisioning whenonCancelis provided, it is omitted otherwise, and clicking invokesonCancelexactly once while showing the disabled "Cancelling…" label. All 3 tests pass viavitest run.pnpm --filter @posthog/ui typecheckpasses; Biome lint clean on the touched files.Automatic notifications
Created with PostHog Code from an inbox report.