fix(home): run quick actions inline with optimistic task insert#2601
fix(home): run quick actions inline with optimistic task insert#2601k11kirky wants to merge 2 commits into
Conversation
Quick actions on a workstream row/card/detail panel no longer navigate away. The action starts the cloud task in place, optimistically splices the new run into the workstream's task list (tagged with the action label), disables the trigger with a spinner while in flight, and nudges a server snapshot refresh so the next poll reconciles. Threads the quick-action label end-to-end (TaskCreationInput -> saga -> api-client body -> home_quick_action) and renders it: a per-task chip in the detail panel and a glanceable indicator on the row showing which quick actions have run against the workstream. Generated-By: PostHog Code Task-Id: 8dc7acb2-b80c-402c-be8c-5a82ea04a68f
|
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/home/hooks/useRunWorkstreamAction.ts:75-107
**Per-instance `inFlightRef` doesn't guard across simultaneous row + detail panel mounts**
`HomeWorkstreamRow` (via `useWorkstreamPresentation`) and `HomeWorkstreamDetailPanel` both mount their own `useRunWorkstreamAction()` instance, each with an independent `inFlightRef` and `isPending` state. When the detail panel is open alongside the row, clicking the quick action in the row sets only the row's `inFlightRef.current = true` and `isPending = true`; the detail panel's guard is still `false`. A user can then immediately click the panel's action button and start a second task for the same workstream.
Before this PR, navigation away unmounted both surfaces, so races were impossible. The new stay-on-Home design keeps both mounted, making this a real double-submit path. Sharing state through a context or lifting `inFlightRef` to a workstream-keyed store (e.g. a `Map<workstreamId, boolean>` ref at a common ancestor) would close the gap.
### Issue 2 of 3
packages/ui/src/features/home/hooks/useRunWorkstreamAction.ts:197-199
`refreshHome` is the entire `useMutation` result object, which gets a new reference whenever mutation state transitions (idle → pending → settled). Including it in the `useCallback` dep array means `run` is recreated on every transition, which defeats memoisation and propagates new references through every `useWorkstreamPresentation` caller. `mutateAsync` is a stable reference in react-query v5 and is the correct dep here.
```suggestion
queryClient,
refreshHome.mutateAsync,
getUserIntegrationIdForRepo,
```
### Issue 3 of 3
packages/ui/src/features/home/utils/optimisticTask.test.ts:40-71
**Prefer parameterised tests for `workstreamTaskFromTask`**
The four `workstreamTaskFromTask` cases are each a distinct input-to-output mapping and are a natural fit for `it.each`. Per the project's convention, parameterised tests are preferred; this block could be expressed as a single `it.each` table covering id, title, status, and quickAction across the four variants.
Reviews (1): Last reviewed commit: "fix(home): run quick actions inline with..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -87,14 +104,7 @@ export function useRunWorkstreamAction() { | |||
|
|
|||
| if (inFlightRef.current) return; | |||
| inFlightRef.current = true; | |||
|
|
|||
| const pendingTaskKey = | |||
| globalThis.crypto?.randomUUID?.() ?? `pending-${Date.now()}`; | |||
| pendingTaskPromptStoreApi.set(pendingTaskKey, { | |||
| promptText, | |||
| attachments: [], | |||
| }); | |||
| navigateToTaskPending(pendingTaskKey); | |||
| setIsPending(true); | |||
There was a problem hiding this comment.
inFlightRef doesn't guard across simultaneous row + detail panel mounts
HomeWorkstreamRow (via useWorkstreamPresentation) and HomeWorkstreamDetailPanel both mount their own useRunWorkstreamAction() instance, each with an independent inFlightRef and isPending state. When the detail panel is open alongside the row, clicking the quick action in the row sets only the row's inFlightRef.current = true and isPending = true; the detail panel's guard is still false. A user can then immediately click the panel's action button and start a second task for the same workstream.
Before this PR, navigation away unmounted both surfaces, so races were impossible. The new stay-on-Home design keeps both mounted, making this a real double-submit path. Sharing state through a context or lifting inFlightRef to a workstream-keyed store (e.g. a Map<workstreamId, boolean> ref at a common ancestor) would close the gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/home/hooks/useRunWorkstreamAction.ts
Line: 75-107
Comment:
**Per-instance `inFlightRef` doesn't guard across simultaneous row + detail panel mounts**
`HomeWorkstreamRow` (via `useWorkstreamPresentation`) and `HomeWorkstreamDetailPanel` both mount their own `useRunWorkstreamAction()` instance, each with an independent `inFlightRef` and `isPending` state. When the detail panel is open alongside the row, clicking the quick action in the row sets only the row's `inFlightRef.current = true` and `isPending = true`; the detail panel's guard is still `false`. A user can then immediately click the panel's action button and start a second task for the same workstream.
Before this PR, navigation away unmounted both surfaces, so races were impossible. The new stay-on-Home design keeps both mounted, making this a real double-submit path. Sharing state through a context or lifting `inFlightRef` to a workstream-keyed store (e.g. a `Map<workstreamId, boolean>` ref at a common ancestor) would close the gap.
How can I resolve this? If you propose a fix, please make it concise.| queryClient, | ||
| refreshHome, | ||
| getUserIntegrationIdForRepo, |
There was a problem hiding this comment.
refreshHome is the entire useMutation result object, which gets a new reference whenever mutation state transitions (idle → pending → settled). Including it in the useCallback dep array means run is recreated on every transition, which defeats memoisation and propagates new references through every useWorkstreamPresentation caller. mutateAsync is a stable reference in react-query v5 and is the correct dep here.
| queryClient, | |
| refreshHome, | |
| getUserIntegrationIdForRepo, | |
| queryClient, | |
| refreshHome.mutateAsync, | |
| getUserIntegrationIdForRepo, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/home/hooks/useRunWorkstreamAction.ts
Line: 197-199
Comment:
`refreshHome` is the entire `useMutation` result object, which gets a new reference whenever mutation state transitions (idle → pending → settled). Including it in the `useCallback` dep array means `run` is recreated on every transition, which defeats memoisation and propagates new references through every `useWorkstreamPresentation` caller. `mutateAsync` is a stable reference in react-query v5 and is the correct dep here.
```suggestion
queryClient,
refreshHome.mutateAsync,
getUserIntegrationIdForRepo,
```
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!
| describe("workstreamTaskFromTask", () => { | ||
| it("maps a created task to a provisional queued workstream task", () => { | ||
| const wsTask = workstreamTaskFromTask(makeTask()); | ||
| expect(wsTask).toEqual({ | ||
| id: "task_1", | ||
| title: "Fix CI", | ||
| status: "queued", | ||
| isGenerating: false, | ||
| needsPermission: false, | ||
| quickAction: null, | ||
| }); | ||
| }); | ||
|
|
||
| it("records the quick action label when provided", () => { | ||
| expect(workstreamTaskFromTask(makeTask(), "Fix CI").quickAction).toBe( | ||
| "Fix CI", | ||
| ); | ||
| }); | ||
|
|
||
| it("prefers the latest run status when present", () => { | ||
| const wsTask = workstreamTaskFromTask( | ||
| makeTask({ latest_run: { status: "in_progress" } as Task["latest_run"] }), | ||
| ); | ||
| expect(wsTask.status).toBe("in_progress"); | ||
| }); | ||
|
|
||
| it("falls back to a placeholder title", () => { | ||
| expect(workstreamTaskFromTask(makeTask({ title: "" })).title).toBe( | ||
| "New task", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
workstreamTaskFromTask
The four workstreamTaskFromTask cases are each a distinct input-to-output mapping and are a natural fit for it.each. Per the project's convention, parameterised tests are preferred; this block could be expressed as a single it.each table covering id, title, status, and quickAction across the four variants.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/home/utils/optimisticTask.test.ts
Line: 40-71
Comment:
**Prefer parameterised tests for `workstreamTaskFromTask`**
The four `workstreamTaskFromTask` cases are each a distinct input-to-output mapping and are a natural fit for `it.each`. Per the project's convention, parameterised tests are preferred; this block could be expressed as a single `it.each` table covering id, title, status, and quickAction across the four variants.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
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!
Addresses Greptile review on PR #2601. - Move the quick-action in-flight guard from a per-hook ref/state into a shared, workstream-keyed Zustand store. The list/board row and the open detail panel mount independent useRunWorkstreamAction hooks, so the previous per-instance guard let both start a task for the same workstream. The store keys by workstream id, so the same workstream can't double-submit while distinct workstreams still run concurrently. - Depend on refreshHome.mutateAsync (stable in react-query v5) instead of the whole mutation object, so run isn't recreated on every mutation transition. - Parameterise the workstreamTaskFromTask tests with it.each per repo convention. Generated-By: PostHog Code Task-Id: 8dc7acb2-b80c-402c-be8c-5a82ea04a68f
1 participant
Problem
On the Home tab, clicking a quick action ("Fix CI", "Address comments", "Create PR", "Review with agent") navigated away — to the pending route and then the task-detail view — and gave no in-flight feedback on the button. The desired behaviour is to kick off the cloud task in place, show it immediately in the workstream's task list, and disable the button while it starts.
Changes
useRunWorkstreamActionno longer navigates to the pending or task-detail routes. It starts the cloud task, optimistically splices the new run into the workstream's task list (insertOptimisticTask), disables the trigger with a spinner while in flight (isPendingthreaded throughuseWorkstreamPresentationto the row / card / detail-panel buttons and overflow items), and fires a server snapshot refresh so the next poll reconciles. The offline / signed-out fallback still routes to the new-task screen.TaskCreationInput.homeQuickActionLabel→ saga → api-clienthome_quick_actionbody) so the server records which quick action started a run. The snapshot'sHomeWorkstreamTaskgainsquickAction, rendered as a per-task chip in the detail panel and a glanceable "✨ Fix CI, …" indicator on the workstream row. The optimistic row is tagged immediately too.Server half (grouping +
home_quick_actionpersistence +quickActionin the snapshot) is inPostHog/posthog.How did you test this?
I'm an agent (Claude Code). Automated checks run in the sandbox:
vitest run src/features/home— 36 passing, including newoptimisticTaskcoverage (splice into the right workstream, no cross-workstream bleed, no duplicates, quick-action tagging).tsc --noEmitclean across@posthog/ui,@posthog/core,@posthog/shared,@posthog/api-client.biome checkclean on all touched files.I did not run the app manually.
Automatic notifications