From bad35775a1793c8a3960c768b62a8006a7f8a4c6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 10:26:21 -0700 Subject: [PATCH] =?UTF-8?q?Drift=203/4:=20Task=20Detail=20intake=20afforda?= =?UTF-8?q?nces=20from=20traits=20=E2=80=94=20the=20UI=20half=20of=20the?= =?UTF-8?q?=20#2571=20approve/reject=20stall=20(4=E2=86=923)=20(#2577)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Drift conversion 3 of 4 — Task Detail, the UI half of the #2571 stall **Stacks on #2566.** Merge order: #2558 → #2566 → this. (#2571 is the P0 and is independent — merge it first regardless.) ### Convergence number Live-code `column === / !== "todo" | "triage"` in `TaskDetailModal.tsx`: **4 → 3** All three survivors are the documented no-metadata fallback, same shape as TaskCard and ListView: `workflowMoveMetadata` is `null` until the detail payload resolves, and a bare trait read would drop these controls during that window. ### This is the UI half of the P0 `isAwaitingApproval` and the standalone Delete button were both gated on `task.column === "triage"`. On the merged lineage (#2515) that is false for every card, so a task parked `awaiting-approval` **loses its Approve/Reject controls in the one surface that shows them**. #2571 fixes the routes that *reject* those actions. This fixes the UI that stops *offering* them. Either half alone leaves the operator stuck — one with buttons that 400, the other with no buttons at all. ### Three conversions | site | was | now | |---|---|---| | `isAwaitingApproval` + standalone Delete | `column === "triage"` | resolved column's `intake` | | `requiresExecutionModeReplan` | `todo \|\| in-progress` | `hold \|\| countsTowardWip` | | move-progress prompt | source column ids | **target** column's flags | The replan rule is "this card may already hold a plan or a live execution context" — which the traits state directly; `todo`/`in-progress` was the Default workflow's spelling of it. The move prompt is the mistake I made first in TaskCard, where its regression test caught that the site tests the move **destination**, not the card. Carried the lesson here rather than repeating it. ### Tested through a pure seam, and why `requiresExecutionModeReplanForTest` is exported so the rule can be asserted as a function of (column id, flags). Asserting it through the modal means booting async detail loading to observe one boolean — and an earlier DOM-level attempt at exactly this class of assertion (in #2566, ListView) **passed with the conversion reverted**, because the text it matched also appears in a column header. I am not repeating that. A seam discriminates; that DOM test did not. Revert-proof: restore `column === "todo" || column === "in-progress"` and the merged-column case fails, because that column is `intake + hold` and carries no `countsTowardWip`. The suite also pins that the rule still **narrows** (a complete lane needs no replan) and that the legacy fallback is unchanged when flags are absent. ### Verification `pnpm test:gate` (414 + 10 + 71), `pnpm lint`, dashboard typecheck green. **No new failures**: `TaskDetailModal.rendering.test.tsx` reports the same 28 pre-existing failures with and without this change, diffed by test *name* against a stashed clean tree. ### Drift set status | file | before | after | PR | |---|---|---|---| | `TaskCard.tsx` | 8 | 3 | #2558 | | `ListView.tsx` | 5 | 3 | #2566 | | `taskActivity.ts` (found underneath) | 1 | 1 | #2566 | | `TaskDetailModal.tsx` | 4 | 3 | this | | `register-task-workflow-routes.ts` | 10 | 11 | #2571 (P0, widened on purpose) | Survivors are no-metadata fallbacks except the routes, where the guards deliberately accept resolved-intake **or** `triage` so a P0 fix cannot reject anything previously allowed. Those retire together once the legacy id is gone board-wide. --------- Co-authored-by: Claude Opus 5 (1M context) --- packages/dashboard/app/components/Column.tsx | 5 +- .../dashboard/app/components/ListView.tsx | 42 ++++++++++++-- .../dashboard/app/components/TaskCard.tsx | 21 ++++++- .../app/components/TaskDetailModal.tsx | 56 +++++++++++++++--- .../TaskCard.u11-merged-column.test.tsx | 25 +++++++- ...TaskDetailModal.u11-merged-column.test.tsx | 46 +++++++++++++++ .../taskActivity.u11-planner-lane.test.ts | 57 +++++++++++++++++++ packages/dashboard/app/utils/taskActivity.ts | 25 +++++++- 8 files changed, 255 insertions(+), 22 deletions(-) create mode 100644 packages/dashboard/app/components/__tests__/TaskDetailModal.u11-merged-column.test.tsx create mode 100644 packages/dashboard/app/utils/__tests__/taskActivity.u11-planner-lane.test.ts diff --git a/packages/dashboard/app/components/Column.tsx b/packages/dashboard/app/components/Column.tsx index 314c3a7971..7a65735a42 100644 --- a/packages/dashboard/app/components/Column.tsx +++ b/packages/dashboard/app/components/Column.tsx @@ -333,7 +333,10 @@ function ColumnComponent({ column, tasks, projectId, maxConcurrent, showWorktree const activeTaskCount = useMemo( () => tasks.filter((task) => isRunningAgentTask(enrichRunningAgentTaskShapeFromFlags(task, columnFlags)) - || isTaskAgentActive(task, { globalPaused, isStuck: isTaskStuck(task, taskStuckTimeoutMs, lastFetchTimeMs) }), + // FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (PR #2566 review — greptile): these + // tasks are IN this column, so the column's own flags are their column traits. Without + // them the header undercounts executing work on a merged planning lane. + || isTaskAgentActive(task, { globalPaused, isStuck: isTaskStuck(task, taskStuckTimeoutMs, lastFetchTimeMs), columnFlags }), ).length, [tasks, columnFlags, globalPaused, taskStuckTimeoutMs, lastFetchTimeMs], ); diff --git a/packages/dashboard/app/components/ListView.tsx b/packages/dashboard/app/components/ListView.tsx index ef3c2783d6..6783cee3a9 100644 --- a/packages/dashboard/app/components/ListView.tsx +++ b/packages/dashboard/app/components/ListView.tsx @@ -855,6 +855,21 @@ export function ListView({ return null; }, [boardWorkflows, workflowMode]); + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The card's INTAKE role, from its own column's traits. Both grouped-list render paths + gated the transient Planning badge on `task.column === "triage"`, which U11 deletes — + the badge would simply stop appearing on planning rows, with nothing failing. + + ONE fallback, matching TaskCard: `columnFlagsById` has no entry for a column the + resolved workflow does not declare (a stranded card, or the pre-load window), and a + bare trait read would drop the badge there too. + */ + const isIntakeColumnForTask = useCallback((task: Task): boolean => { + const flags = columnFlagsById.get(task.column); + return flags ? flags.intake === true : task.column === "triage"; + }, [columnFlagsById]); + const isArchivedColumn = useCallback((column: ColumnId): boolean => { return workflowMode ? Boolean(columnFlagsById.get(column)?.archived) : column === "archived"; }, [columnFlagsById, workflowMode]); @@ -1903,8 +1918,19 @@ export function ListView({ try { const hasStepProgress = task.steps.some((step) => step.status !== "pending"); const targetFlags = columnFlagsById.get(column); + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + Flags FIRST, ids only when the destination has no resolved metadata. The previous + form OR-ed the two, so a column merely NAMED `todo` or `triage` prompted regardless + of its traits — and post-U11 that is the merged column's id, meaning the legacy + disjunct would keep firing for reasons unrelated to what the column IS. Reading the + traits when they exist makes the rule mean "moving back into a pre-implementation + lane", which is the thing worth warning about. + */ const shouldPrompt = hasStepProgress && ( - column === "todo" || column === "triage" || Boolean(targetFlags?.intake || targetFlags?.hold) + targetFlags + ? Boolean(targetFlags.intake || targetFlags.hold) + : column === "todo" || column === "triage" ); let moveOptions: { preserveProgress?: boolean } | undefined; @@ -2451,8 +2477,12 @@ export function ListView({ const task = tasks.find((candidate) => candidate.id === taskId); const hasStepProgress = task?.steps.some((step) => step.status !== "pending") ?? false; const targetFlags = columnFlagsById.get(column); + // Same rule as the context-menu move above: flags first, ids only as the + // no-metadata fallback. const shouldPrompt = hasStepProgress && ( - column === "todo" || column === "triage" || Boolean(targetFlags?.intake || targetFlags?.hold) + targetFlags + ? Boolean(targetFlags.intake || targetFlags.hold) + : column === "todo" || column === "triage" ); let moveOptions: { preserveProgress?: boolean } | undefined; @@ -2941,9 +2971,9 @@ export function ListView({ const isFailed = !isDoneColumn && task.status === "failed" && !hasPendingAutomaticRecovery(task, lastFetchTimeMs); const isPaused = !isDoneColumn && task.paused === true; const isStuckState = isTaskStuck(task, taskStuckTimeoutMs, lastFetchTimeMs); - const isAgentActive = isTaskAgentActive(task, { globalPaused, isStuck: isStuckState }); + const isAgentActive = isTaskAgentActive(task, { globalPaused, isStuck: isStuckState, columnFlags: columnFlagsById.get(task.column) }); // FNXC:TaskStatusBadge 2026-07-28-12:00: FN-8300 renders the same transient Planning badge as TaskCard so fresh planner logs never make grouped-list cards appear idle. - const isTransientPlannerActive = task.column === "triage" + const isTransientPlannerActive = isIntakeColumnForTask(task) && !visualStatus && Boolean(task.recentAgentActivityAt) && isAgentActive; @@ -3204,9 +3234,9 @@ export function ListView({ const isFailed = !isDoneColumn && task.status === "failed" && !hasPendingAutomaticRecovery(task, lastFetchTimeMs); const isPaused = !isDoneColumn && task.paused === true; const isStuckState = isTaskStuck(task, taskStuckTimeoutMs, lastFetchTimeMs); - const isAgentActive = isTaskAgentActive(task, { globalPaused, isStuck: isStuckState }); + const isAgentActive = isTaskAgentActive(task, { globalPaused, isStuck: isStuckState, columnFlags: columnFlagsById.get(task.column) }); const isReviewBudgetExhausted = isReviewBudgetExhaustedApproval(task); - const isTransientPlannerActive = task.column === "triage" + const isTransientPlannerActive = isIntakeColumnForTask(task) && !visualStatus && Boolean(task.recentAgentActivityAt) && isAgentActive; diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index c8ef3adecd..bbb54f6bbc 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -1433,7 +1433,15 @@ function TaskCardComponent({ const isPlanReviewReplanCapApproval = isReviewBudgetExhaustedApproval(task); const isAwaitingInput = task.status === "awaiting-user-input"; const isArchived = task.column === "archived"; - const isAgentActive = isTaskAgentActive(task, { globalPaused, queued, isStuck }); + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (PR #2566 review — greptile): + Pass the card's column traits. Without them the planner-lane clause falls back to the + legacy ids, so a status-null card on the MERGED planning lane (id `todo`, intake+hold) + is not recognised as having fresh planner activity: the pulsing Planning state, the + optional-gate activity and the column's executing count all read idle. Threading + ListView alone left this path — the board cards — still broken. + */ + const isAgentActive = isTaskAgentActive(task, { globalPaused, queued, isStuck, columnFlags: taskColumnFlags }); /* FNXC:TaskCardOptionalGateBadge 2026-07-21-22:30: Match FN-8055: optional-gate badges pulse only while the card is agent-active (queue/pause/stuck gates suppress the badge). @@ -2505,7 +2513,16 @@ function TaskCardComponent({ } catch (err) { addToast(getErrorMessage(err), "error"); } - }, [addToast, columnLabel, confirm, onMoveTask, task.id, task.steps, t]); + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (PR #2566 review — greptile): + `taskMoveColumns` MUST be a dependency. The prompt now resolves the target column's + traits from it, so omitting it pins the callback to whatever metadata existed at first + render: once the board-workflows payload arrives or changes, a move into a custom + intake/hold lane would skip the preserve-progress confirmation entirely (silently + resetting work), while stale traits could prompt for a lane that is no longer + pre-implementation. I added the lookup and missed the dep. + */ + }, [addToast, columnLabel, confirm, onMoveTask, task.id, task.steps, t, taskMoveColumns]); const handleTaskActionCheckPrStatus = useCallback(async () => { try { diff --git a/packages/dashboard/app/components/TaskDetailModal.tsx b/packages/dashboard/app/components/TaskDetailModal.tsx index 7b52314926..ae90720aca 100644 --- a/packages/dashboard/app/components/TaskDetailModal.tsx +++ b/packages/dashboard/app/components/TaskDetailModal.tsx @@ -537,14 +537,31 @@ function normalizeExecutionModeValue(executionMode: Task["executionMode"]): "sta return executionMode === "fast" ? "fast" : "standard"; } -function requiresExecutionModeReplan(column: Task["column"]): boolean { +function requiresExecutionModeReplan(column: Task["column"], flags?: TaskContextMenuColumnFlags): boolean { /* FNXC:ExecutionModeReplan 2026-06-30-00:00: Todo and in-progress tasks can already hold a generated plan or active execution context. Changing Standard/Fast mode invalidates that plan, so the dashboard must confirm the change and send the task back through the existing replanning path instead of silently patching executionMode in place. + + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The rule is "this card may already hold a plan or a live execution context", which the + traits state directly: a HOLD lane (planned, waiting for capacity) or a WIP lane + (executing). Naming `todo` and `in-progress` was the Default workflow's spelling of + that, and it silently narrows to nothing useful on a renamed workflow. Legacy ids + remain the fallback for callers without resolved column metadata. */ + if (flags) return flags.hold === true || flags.countsTowardWip === true; return column === "todo" || column === "in-progress"; } +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +Test seam. The rule is a pure function of (column id, column flags), and asserting it +through the full modal means booting async detail loading to observe one boolean — an +earlier DOM-level attempt at this class of assertion in ListView passed with the +conversion reverted, because the text it matched also appears in a column header. +*/ +export const requiresExecutionModeReplanForTest = requiresExecutionModeReplan; + interface ProvenanceDisplay { label: string; labelHref?: string; @@ -2023,7 +2040,7 @@ export function TaskDetailContent({ } return false; } - const replanAfterExecutionModeChange = Object.prototype.hasOwnProperty.call(updates, "executionMode") && requiresExecutionModeReplan(task.column); + const replanAfterExecutionModeChange = Object.prototype.hasOwnProperty.call(updates, "executionMode") && requiresExecutionModeReplan(task.column, workflowMoveMetadata?.currentColumnFlags); if (replanAfterExecutionModeChange && !includeDescription) { delete updates.executionMode; } @@ -2161,7 +2178,7 @@ export function TaskDetailContent({ const currentMode = normalizeExecutionModeValue(task.executionMode); const nextMode = currentMode === "fast" ? "standard" : "fast"; const previousMode = inlineExecutionMode; - const shouldReplan = requiresExecutionModeReplan(task.column); + const shouldReplan = requiresExecutionModeReplan(task.column, workflowMoveMetadata?.currentColumnFlags); if (shouldReplan) { const shouldChangeMode = await confirm({ @@ -2510,7 +2527,18 @@ export function TaskDetailContent({ async (column: Column) => { try { const hasStepProgress = task.steps.some((step) => step.status !== "pending"); - const shouldPrompt = (column === "todo" || column === "triage") && hasStepProgress; + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The TARGET column's role, not this card's — moving BACK into a pre-implementation + lane is what risks discarding step progress. (The same site in TaskCard is where I + first got this backwards; its regression test caught it.) Falls back to the legacy + ids when the destination has no resolved metadata. + */ + const targetFlags = workflowMoveMetadata?.moveColumns?.find((candidate) => candidate.id === column)?.flags; + const targetIsPreImplementation = targetFlags + ? targetFlags.intake === true || targetFlags.hold === true + : column === "todo" || column === "triage"; + const shouldPrompt = targetIsPreImplementation && hasStepProgress; let moveOptions: { preserveProgress?: boolean } | undefined; if (shouldPrompt) { @@ -3006,7 +3034,16 @@ export function TaskDetailContent({ plan-review-replan-cap, explain that Plan Review exhausted automatic REVISE replans without converging so the operator is not guessing why the task is parked. */ - const isAwaitingApproval = task.column === "triage" && task.status === "awaiting-approval"; + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The INTAKE lane's approval hold. `task.column === "triage"` is deleted by U11, which + would silently drop the Approve/Reject controls from a parked planning card — the + operator sees a task stuck "awaiting approval" with no way to answer it. + */ + const isIntakeColumn = workflowMoveMetadata?.currentColumnFlags + ? workflowMoveMetadata.currentColumnFlags.intake === true + : task.column === "triage"; + const isAwaitingApproval = isIntakeColumn && task.status === "awaiting-approval"; const isPlanReviewReplanCapApproval = isReviewBudgetExhaustedApproval(task); const handleTogglePause = useCallback(async () => { @@ -6390,10 +6427,11 @@ export function TaskDetailContent({ )} - {/* Standalone Delete button for triage-column tasks — triage tasks - hide the Actions dropdown (see condition below) so the user has - no quick way to delete a freshly-created task otherwise. */} - {task.column === "triage" && !isAwaitingApproval && !canRetryTask && ( + {/* Standalone Delete button for INTAKE-lane tasks — they hide the Actions + dropdown (see condition below) so the user has no quick way to delete a + freshly-created task otherwise. Keyed on the intake trait rather than the + `triage` id, which U11 deletes. */} + {isIntakeColumn && !isAwaitingApproval && !canRetryTask && (