U12 part 5: put real workflow adjacency on the wire — custom-workflow move menus were guessing (measured), and the VALID_TRANSITIONS shortcut is gone - #2525
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWorkflow columns now expose resolved ChangesWorkflow move metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BoardWorkflowsRoute
participant ListView
participant TaskContextMenu
participant WorkflowColumns
BoardWorkflowsRoute->>WorkflowColumns: resolve moveTargets
WorkflowColumns-->>ListView: workflow columns with moveTargets
ListView->>TaskContextMenu: build actions for task
TaskContextMenu->>TaskContextMenu: select task workflow metadata
TaskContextMenu->>TaskContextMenu: filter to visible destinations
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR puts workflow-resolved move adjacency into the board-workflows payload and consumes it across task move menus.
Confidence Score: 3/5The PR is not yet safe to merge because ListView still leaves newly arrived unmapped tasks with guessed move targets until workflow metadata refreshes. The per-task mapping fixes cross-workflow adjacency once metadata is current, but an SSE task that arrives before the board-workflows payload is skipped by the new map and falls back to adjacency-free shared columns; without Board's forced refresh behavior in ListView, the menu can continue offering rejected moves and hiding legal ones. Files Needing Attention: packages/dashboard/app/components/ListView.tsx
|
| Filename | Overview |
|---|---|
| packages/dashboard/src/routes/board-workflows.ts | Adds graph-resolved adjacency to each serialized workflow column. |
| packages/dashboard/app/components/TaskContextMenu.tsx | Uses declared visible adjacency when available and preserves the compatibility fallback. |
| packages/dashboard/app/components/ListView.tsx | Correctly resolves adjacency per task for mapped workflows, but unmapped SSE-new tasks still retain the previously reported stale-menu behavior. |
| packages/dashboard/app/components/Board.tsx | Threads per-workflow adjacency into board task context menus. |
| packages/dashboard/app/components/TaskDetailModal.tsx | Threads resolved adjacency into Task Detail move metadata. |
| packages/dashboard/app/components/tests/workflow-resolved-columns.test.tsx | Covers custom graph adjacency and filtering of unavailable destinations. |
| packages/core/src/tests/builtin-adjacency-matches-legacy-transitions.test.ts | Pins built-in adjacency membership and ordering against legacy transitions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
IR[Workflow IR] --> Resolver[resolveAllowedColumns]
Resolver --> Payload[board-workflows moveTargets]
Payload --> Surfaces[Board / Lane / List / Task Detail]
Surfaces --> Menu[Task move menu]
Menu --> Validation[moveTaskInternal validation]
NewTask[Task SSE arrives before payload refresh] --> Missing[Missing taskWorkflowIds entry]
Missing --> Fallback[Shared union and neighbor fallback]
Fallback --> Menu
Reviews (6): Last reviewed commit: "test(dashboard): exercise a genuinely HI..." | Re-trigger Greptile
…ard (PR #2525 review) Valid finding. In the "All workflows" view `listColumns` is a UNION across workflows keyed by column id, so two workflows that reuse an id but declare different edges collapse into one entry — and `listContextMenuColumns` handed every task the FIRST workflow's `moveTargets`. That offers moves the store will reject and hides legal ones, which is the exact defect this PR exists to remove, reintroduced one layer up. Adjacency is per-workflow, so it must be resolved per task. Added `taskContextMenuColumnsByTaskId`, mirroring the pattern Board already uses (`taskContextMenuColumnsByTaskId` there), so each card gets ITS OWN workflow's columns and edges. The shared union keeps labels and flags only, where collapsing ids is harmless, and remains the fallback when a task's workflow cannot be resolved — which yields the previous neighbour-approximated behaviour rather than a wrong answer. Board was already correct here; only the List aggregate view conflated them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
239074d to
9242ab7
Compare
…view) Second-round finding, valid. `buildBoardWorkflowsPayload` writes a `taskWorkflowIds` entry for EVERY task it is given, null selection included — so a MISSING entry does not mean "no selection", it means this task is NEWER than the payload. That happens routinely: the SSE task list updates before board-workflows does. Falling back to `defaultWorkflowId` there asserted the default workflow's adjacency on a card that may belong to another workflow entirely. Wrong, confidently, for exactly the cards most likely to be affected — freshly created ones, which is precisely when a workflow was chosen. An unmapped task now gets no per-task metadata at all and falls back to the shared union plus the neighbour approximation: the pre-existing behaviour, and an admitted guess rather than a false claim. A PRESENT but unknown id (stale or deleted workflow) still falls back to the default, because there the entry is a real answer that has merely gone out of date. Board additionally forces one board-workflows refetch when it notices unmapped rendered tasks (FN-7591). Porting that self-heal to List is a real improvement and its own change; this one stops List asserting something it does not know. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/dashboard/src/routes/board-workflows.ts (1)
136-146: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid rebuilding column adjacency per column.
In
describeColumns,resolveAllowedColumns(ir, col.id)callsresolveColumnAdjacency(ir)for every column, so the.map()recomputes the full adjacency instead of reusing one prebuilt map. ImportresolveColumnAdjacencyandColumnAdjacencyand pass it intodescribeColumns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/routes/board-workflows.ts` around lines 136 - 146, Update describeColumns to accept a prebuilt ColumnAdjacency argument, import ColumnAdjacency and resolveColumnAdjacency, and construct the adjacency map once before mapping columns. Pass the shared adjacency to each resolveAllowedColumns call instead of rebuilding it per column, and update callers to provide the precomputed map.packages/core/src/__tests__/builtin-adjacency-matches-legacy-transitions.test.ts (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: compare key sets, not counts.
A count check passes if a column is renamed on one side and another added on the other. Comparing sorted keys pins the same property more directly.
♻️ Proposed tweak
- it("covers every legacy column, so a new one cannot slip past this pin", () => { - expect(COLUMNS.length).toBe(Object.keys(VALID_TRANSITIONS).length); - }); + it("covers every legacy column, so a new one cannot slip past this pin", () => { + expect(Object.keys(VALID_TRANSITIONS).sort()).toEqual([...COLUMNS].sort()); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/builtin-adjacency-matches-legacy-transitions.test.ts` around lines 33 - 35, Update the test case that currently compares COLUMNS.length with Object.keys(VALID_TRANSITIONS).length to compare the sorted column names against the sorted VALID_TRANSITIONS keys instead, ensuring both collections contain the same keys rather than merely the same count.packages/dashboard/app/components/ListView.tsx (1)
778-835: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: resolve per-task columns lazily instead of materialising a Map keyed by every task.
The
byWorkflowIdindex is the only real work; the second loop just copies shared references into a per-task Map that is rebuilt on everytasksidentity change (every SSE refresh), and it is read one task at a time inbuildListContextMenuActions. A callback overbyWorkflowIdkeeps identical semantics — including theassigned === undefinedskip — without the O(tasks) rebuild.♻️ Sketch
- const taskContextMenuColumnsByTaskId = useMemo(() => { - const map = new Map<string, readonly TaskContextMenuColumnMetadata[]>(); - if (!workflowMode || !boardWorkflows) return map; + const contextMenuColumnsByWorkflowId = useMemo(() => { const byWorkflowId = new Map<string, readonly TaskContextMenuColumnMetadata[]>(); + if (!workflowMode || !boardWorkflows) return byWorkflowId; for (const workflow of boardWorkflows.workflows) { /* unchanged */ } - for (const task of tasks) { /* … */ } - return map; - }, [boardWorkflows, tasks, workflowMode]); + return byWorkflowId; + }, [boardWorkflows, workflowMode]); + + const resolveTaskContextMenuColumns = useCallback((taskId: string) => { + const assigned = boardWorkflows?.taskWorkflowIds[taskId]; + if (assigned === undefined) return undefined; + const workflowId = contextMenuColumnsByWorkflowId.has(assigned) ? assigned : boardWorkflows?.defaultWorkflowId; + return workflowId ? contextMenuColumnsByWorkflowId.get(workflowId) : undefined; + }, [boardWorkflows, contextMenuColumnsByWorkflowId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/ListView.tsx` around lines 778 - 835, Refactor taskContextMenuColumnsByTaskId to lazily resolve columns per task instead of materialising a Map for every task on each tasks change. Preserve the existing byWorkflowId index, workflowMode/boardWorkflows guards, assigned === undefined skip, validation of stale workflow IDs through defaultWorkflowId, and undefined result when no columns exist; update buildListContextMenuActions to use the resolver callback while keeping current behavior.packages/i18n/locales/en/app.json (1)
7831-7831: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
backTois inserted afterbackToInProgress, breaking the alphabetical key order of themoveblock. Every locale received the new key at the same non-sorted position;backTosorts beforebackToInProgress. Harmless at runtime, but it will churn against any catalog sort check.
packages/i18n/locales/en/app.json#L7831-L7831: move"backTo"above"backToInProgress".packages/i18n/locales/es/app.json#L7794-L7794: same reorder.packages/i18n/locales/fr/app.json#L7794-L7794: same reorder.packages/i18n/locales/ko/app.json#L7794-L7794: same reorder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/i18n/locales/en/app.json` at line 7831, Reorder the move-block keys so backTo appears immediately before backToInProgress in packages/i18n/locales/en/app.json (7831-7831), packages/i18n/locales/es/app.json (7794-7794), packages/i18n/locales/fr/app.json (7794-7794), and packages/i18n/locales/ko/app.json (7794-7794), preserving all translations and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/core/src/__tests__/builtin-adjacency-matches-legacy-transitions.test.ts`:
- Around line 33-35: Update the test case that currently compares COLUMNS.length
with Object.keys(VALID_TRANSITIONS).length to compare the sorted column names
against the sorted VALID_TRANSITIONS keys instead, ensuring both collections
contain the same keys rather than merely the same count.
In `@packages/dashboard/app/components/ListView.tsx`:
- Around line 778-835: Refactor taskContextMenuColumnsByTaskId to lazily resolve
columns per task instead of materialising a Map for every task on each tasks
change. Preserve the existing byWorkflowId index, workflowMode/boardWorkflows
guards, assigned === undefined skip, validation of stale workflow IDs through
defaultWorkflowId, and undefined result when no columns exist; update
buildListContextMenuActions to use the resolver callback while keeping current
behavior.
In `@packages/dashboard/src/routes/board-workflows.ts`:
- Around line 136-146: Update describeColumns to accept a prebuilt
ColumnAdjacency argument, import ColumnAdjacency and resolveColumnAdjacency, and
construct the adjacency map once before mapping columns. Pass the shared
adjacency to each resolveAllowedColumns call instead of rebuilding it per
column, and update callers to provide the precomputed map.
In `@packages/i18n/locales/en/app.json`:
- Line 7831: Reorder the move-block keys so backTo appears immediately before
backToInProgress in packages/i18n/locales/en/app.json (7831-7831),
packages/i18n/locales/es/app.json (7794-7794), packages/i18n/locales/fr/app.json
(7794-7794), and packages/i18n/locales/ko/app.json (7794-7794), preserving all
translations and values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79030d3f-7f7c-4aba-9787-1fbbdc46bb82
📒 Files selected for processing (19)
.changeset/u12-context-menu-back-to-label.md.changeset/u12-move-targets-on-wire.mdpackages/core/src/__tests__/builtin-adjacency-matches-legacy-transitions.test.tspackages/dashboard/app/api/board-workflows.tspackages/dashboard/app/components/Board.tsxpackages/dashboard/app/components/Lane.tsxpackages/dashboard/app/components/ListView.tsxpackages/dashboard/app/components/TaskContextMenu.tsxpackages/dashboard/app/components/TaskDetailModal.tsxpackages/dashboard/app/components/__tests__/ListView.test.tsxpackages/dashboard/app/components/__tests__/workflow-resolved-columns.test.tsxpackages/dashboard/src/routes/board-workflows.tspackages/i18n/locales/en/app.jsonpackages/i18n/locales/es/app.jsonpackages/i18n/locales/fr/app.jsonpackages/i18n/locales/ko/app.jsonpackages/i18n/locales/zh-CN/app.jsonpackages/i18n/locales/zh-TW/app.jsonpackages/i18n/src/resources.d.ts
…VALID_TRANSITIONS shortcut (U12)
The move menu had NO adjacency, so it did two wrong things at once: it approximated
targets from a column's NEIGHBOURS in declared order, and — because that approximation
is strictly weaker than the graph — it kept a `VALID_TRANSITIONS` shortcut for any
workflow whose column-id set matched the six built-ins.
Measured, the approximation loses real operator moves:
in-progress graph: in-review, todo, triage, done neighbours: todo, in-review
todo graph: in-progress, triage, archived neighbours: triage, in-progress
done graph: todo, triage, archived neighbours: in-review, archived
So every CUSTOM workflow has been offering a guess: menu entries the store would
reject, and legal moves it never offered.
`BoardWorkflowColumn` now carries `moveTargets`, resolved by `resolveAllowedColumns` —
the same resolver `moveTaskInternal` validates against — so the menu offers exactly
what the store accepts, for any workflow. Threaded through all four metadata builders
(Board, Lane, ListView, TaskDetailModal). Optional on the wire: a client predating the
field keeps the neighbour fallback rather than losing its menu.
With adjacency available the legacy shortcut is not merely removable, it is redundant:
`resolveAllowedColumns(BUILTIN_CODING_WORKFLOW_IR, c)` is identical to
`VALID_TRANSITIONS[c]` for all six columns, ORDER included. Measured, then pinned by
`builtin-adjacency-matches-legacy-transitions.test.ts` so the equivalence cannot drift
silently — it compares order too, since the menu renders targets in the order it gets
them. Default-workflow menus are therefore byte-identical; custom ones stop guessing.
`COLUMNS` is now unused in TaskContextMenu and its import is gone. `VALID_TRANSITIONS`
survives ONLY for the no-metadata load window, which is documented at the site and
retires when the load window itself does.
Revert-proof, two ways. Drop the `declaredTargets` branch and the custom-workflow case
fails — the neighbour fallback returns ["backlog","building"], missing the legal
`shipped` jump and offering `backlog`, which that graph forbids. A second case pins
that an adjacency edge into a column the board cannot show is dropped rather than
rendered as a dead entry.
Measured: no new test failures. Five suites report 31 failures with and without the
change — an identical, pre-existing set, verified by diffing failing test NAMES against
a stashed clean tree rather than comparing counts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ard (PR #2525 review) Valid finding. In the "All workflows" view `listColumns` is a UNION across workflows keyed by column id, so two workflows that reuse an id but declare different edges collapse into one entry — and `listContextMenuColumns` handed every task the FIRST workflow's `moveTargets`. That offers moves the store will reject and hides legal ones, which is the exact defect this PR exists to remove, reintroduced one layer up. Adjacency is per-workflow, so it must be resolved per task. Added `taskContextMenuColumnsByTaskId`, mirroring the pattern Board already uses (`taskContextMenuColumnsByTaskId` there), so each card gets ITS OWN workflow's columns and edges. The shared union keeps labels and flags only, where collapsing ids is harmless, and remains the fallback when a task's workflow cannot be resolved — which yields the previous neighbour-approximated behaviour rather than a wrong answer. Board was already correct here; only the List aggregate view conflated them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ove metadata (PR #2528 review) Valid finding. The per-task lookup used `taskWorkflowIds[task.id] ?? defaultWorkflowId`, which covers the MISSING case but not the INVALID one. `taskWorkflowIds` can carry a stale or unknown entry — a workflow deleted since the payload was built, or an id this client has not seen — and an unknown id resolves to no columns, so the task silently drops back to the adjacency-free shared union. The menu is then wrong in exactly the way this change exists to prevent, and wrong most often for the tasks whose workflow state is already unusual. Now validated against the known-workflow set before use, falling back to the default otherwise. This mirrors Board's `getEffectiveTaskWorkflowId`, which already validates for the same reason — I copied the shape of that resolver without copying its guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…view) Second-round finding, valid. `buildBoardWorkflowsPayload` writes a `taskWorkflowIds` entry for EVERY task it is given, null selection included — so a MISSING entry does not mean "no selection", it means this task is NEWER than the payload. That happens routinely: the SSE task list updates before board-workflows does. Falling back to `defaultWorkflowId` there asserted the default workflow's adjacency on a card that may belong to another workflow entirely. Wrong, confidently, for exactly the cards most likely to be affected — freshly created ones, which is precisely when a workflow was chosen. An unmapped task now gets no per-task metadata at all and falls back to the shared union plus the neighbour approximation: the pre-existing behaviour, and an admitted guess rather than a false claim. A PRESENT but unknown id (stale or deleted workflow) still falls back to the default, because there the entry is a real answer that has merely gone out of date. Board additionally forces one board-workflows refetch when it notices unmapped rendered tasks (FN-7591). Porting that self-heal to List is a real improvement and its own change; this one stops List asserting something it does not know. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3af6cbe to
8324939
Compare
… a shared hook MOVE ONLY — no behaviour change. Preparing to give ListView the same self-heal without growing a second copy of it. Board carries the FN-7591 invariant: when a rendered task's `taskWorkflowIds` mapping is absent or suspect (present, but resolving to a workflow that does not declare the task's stored column), force ONE board-workflows refetch so the real selection resolves. Signature-guarded against refetch loops, deferred one macrotask so an optimistic workflow seed lands first. ListView needs exactly this and had none of it — the gap greptile raised on #2525, which I said there I would fix separately rather than inline. Evidence the move is a move: with comments and the new wrapper signature stripped, the hook's 41 body lines and the 42 removed from Board differ by exactly one line — the `}` that closed Board's enclosing scope. Nothing else was added, removed or reordered. The original FNXC notes travel with the code, since they are the reason every line exists. Board's suite is green with no expectation edits (94 passed across Board and the cross-surface workflow-selection suite). ListView is wired to this hook in the NEXT commit, so the move and the behaviour change stay separable and independently revertable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l (PR #2525 review) Behaviour change, on top of the previous commit's verbatim extraction. A task whose `taskWorkflowIds` entry is absent — or present but resolving to a workflow that does not declare the task's stored column — gets no per-workflow move metadata, so its menu falls back to the neighbour approximation and STAYS there until some unrelated refresh happens. Board has forced one board-workflows refetch for this since FN-7591; List had none. The degraded state therefore persisted longest exactly where it is most likely: a just-created card, which is precisely when a workflow was chosen. Revert-proof: remove the hook call from ListView and the new case fails — `fetchBoardWorkflows` is never called a second time, so the mapping never resolves. A companion case pins the other half, that a fully-mapped board does NOT refetch, so the signature guard cannot turn a healthy list into a refetch loop. It measures calls made AFTER the initial load settles, because other mechanisms (mount fetch, switcher open) legitimately call the fetcher and counting from zero would measure them instead. Two existing tests needed their fixtures corrected, both because the self-heal now fires CORRECTLY where they did not expect a fetch: - "refreshes workflow columns when workflow metadata SSE arrives" chained two `mockResolvedValueOnce` payloads. The file-level cache seed maps no tasks, so first paint saw FN-001 unmapped and the repair fetch ate the payload the test asserts on. Seeded that test's own first-paint cache, and added a trailing default because the SSE swap (`backlog` -> `ready`) leaves FN-001 in a column its workflow no longer declares — a repair fetch there is right, and without a fallback it resolved `undefined` and wiped the payload. Neither was a behaviour regression; both were fixtures that had quietly depended on List never self-healing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/TaskContextMenu.tsx (1)
237-241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail closed while workflow adjacency is unavailable.
Legacy transitions can disagree with a selected custom workflow that reuses built-in column IDs, exposing move actions the workflow graph did not authorize. Do not infer targets until resolved graph metadata is available.
packages/dashboard/app/components/TaskContextMenu.tsx#L237-L241: return no move transitions (or a non-actionable loading state) whenworkflowMoveColumnsis absent.packages/dashboard/app/components/TaskContextMenu.tsx#L184-L187: distinguish absentmoveTargetsfrom an empty adjacency and do not fall through to neighbor-based targets.packages/dashboard/app/components/TaskContextMenu.tsx#L6-L10: removeVALID_TRANSITIONSand its fallback rationale once graph-derived actions fail closed.As per coding guidelines, “workflow graph ownership is unconditional and missing workflow selection must fail closed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/TaskContextMenu.tsx` around lines 237 - 241, Make workflow graph ownership unconditional in TaskContextMenu: at packages/dashboard/app/components/TaskContextMenu.tsx lines 237-241, return no move transitions when workflowMoveColumns is absent instead of falling back to VALID_TRANSITIONS; at lines 184-187, distinguish absent moveTargets from an empty adjacency and avoid neighbor-based targets when absent; at lines 6-10, remove the now-unused VALID_TRANSITIONS import and fallback rationale.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/dashboard/app/components/__tests__/workflow-resolved-columns.test.tsx`:
- Around line 536-550: Update the test around getTaskMoveTransitions to include
a “nowhere” entry in withHidden with hiddenFromBoard: true, while retaining it
in staging.moveTargets. Assert that the resulting transitions still exclude that
known hidden column, so the test covers hidden-target filtering rather than only
unknown IDs.
---
Outside diff comments:
In `@packages/dashboard/app/components/TaskContextMenu.tsx`:
- Around line 237-241: Make workflow graph ownership unconditional in
TaskContextMenu: at packages/dashboard/app/components/TaskContextMenu.tsx lines
237-241, return no move transitions when workflowMoveColumns is absent instead
of falling back to VALID_TRANSITIONS; at lines 184-187, distinguish absent
moveTargets from an empty adjacency and avoid neighbor-based targets when
absent; at lines 6-10, remove the now-unused VALID_TRANSITIONS import and
fallback rationale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84e84cea-2975-4c6e-9274-a02f17d8917f
📒 Files selected for processing (10)
.changeset/u12-move-targets-on-wire.mdpackages/core/src/__tests__/builtin-adjacency-matches-legacy-transitions.test.tspackages/dashboard/app/api/board-workflows.tspackages/dashboard/app/components/Board.tsxpackages/dashboard/app/components/Lane.tsxpackages/dashboard/app/components/ListView.tsxpackages/dashboard/app/components/TaskContextMenu.tsxpackages/dashboard/app/components/TaskDetailModal.tsxpackages/dashboard/app/components/__tests__/workflow-resolved-columns.test.tsxpackages/dashboard/src/routes/board-workflows.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/tests/builtin-adjacency-matches-legacy-transitions.test.ts
- packages/dashboard/app/api/board-workflows.ts
- packages/dashboard/src/routes/board-workflows.ts
- .changeset/u12-move-targets-on-wire.md
- packages/dashboard/app/components/Board.tsx
- packages/dashboard/app/components/Lane.tsx
- packages/dashboard/app/components/TaskDetailModal.tsx
- packages/dashboard/app/components/ListView.tsx
…own id (PR #2525 review) Valid. The case targeted `nowhere`, an id the workflow does not declare at all — so it proved the unknown-id path and left the `hiddenFromBoard` filtering untested. Deleting the visibility filter would have kept it green. Now both, because they fail differently: `hidden` is a DECLARED column carrying `hiddenFromBoard` that only the visibility filter removes, and `nowhere` is undeclared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a shared hook MOVE ONLY — no behaviour change. Preparing to give ListView the same self-heal without growing a second copy of it. Board carries the FN-7591 invariant: when a rendered task's `taskWorkflowIds` mapping is absent or suspect (present, but resolving to a workflow that does not declare the task's stored column), force ONE board-workflows refetch so the real selection resolves. Signature-guarded against refetch loops, deferred one macrotask so an optimistic workflow seed lands first. ListView needs exactly this and had none of it — the gap greptile raised on #2525, which I said there I would fix separately rather than inline. Evidence the move is a move: with comments and the new wrapper signature stripped, the hook's 41 body lines and the 42 removed from Board differ by exactly one line — the `}` that closed Board's enclosing scope. Nothing else was added, removed or reordered. The original FNXC notes travel with the code, since they are the reason every line exists. Board's suite is green with no expectation edits (94 passed across Board and the cross-surface workflow-selection suite). ListView is wired to this hook in the NEXT commit, so the move and the behaviour change stay separable and independently revertable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l (PR #2525 review) Behaviour change, on top of the previous commit's verbatim extraction. A task whose `taskWorkflowIds` entry is absent — or present but resolving to a workflow that does not declare the task's stored column — gets no per-workflow move metadata, so its menu falls back to the neighbour approximation and STAYS there until some unrelated refresh happens. Board has forced one board-workflows refetch for this since FN-7591; List had none. The degraded state therefore persisted longest exactly where it is most likely: a just-created card, which is precisely when a workflow was chosen. Revert-proof: remove the hook call from ListView and the new case fails — `fetchBoardWorkflows` is never called a second time, so the mapping never resolves. A companion case pins the other half, that a fully-mapped board does NOT refetch, so the signature guard cannot turn a healthy list into a refetch loop. It measures calls made AFTER the initial load settles, because other mechanisms (mount fetch, switcher open) legitimately call the fetcher and counting from zero would measure them instead. Two existing tests needed their fixtures corrected, both because the self-heal now fires CORRECTLY where they did not expect a fetch: - "refreshes workflow columns when workflow metadata SSE arrives" chained two `mockResolvedValueOnce` payloads. The file-level cache seed maps no tasks, so first paint saw FN-001 unmapped and the repair fetch ate the payload the test asserts on. Seeded that test's own first-paint cache, and added a trailing default because the SSE swap (`backlog` -> `ready`) leaves FN-001 in a column its workflow no longer declares — a repair fetch there is right, and without a fallback it resolved `undefined` and wiped the payload. Neither was a behaviour regression; both were fixtures that had quietly depended on List never self-healing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e inline arrow, measured with a memo-comparator probe (#2528) ## U12 part 6 — Board re-rendered every column on every state change **Stacks on #2525.** Merge that first. `canDropTask` was allocated as a fresh inline arrow, per column, per render: ```tsx canDropTask={(taskId) => canDropTask(taskId, columnDef.id, selectedWorkflow.id)} ``` `Column` is `React.memo`, and a new function identity on any prop defeats that entirely. So **any** Board state change — collapsing Archived, changing Done sort, opening the workflow switcher — re-rendered every column and every card beneath it, not just the affected one. Bound through a `useMemo` cache keyed by lane + column. After the fix, collapsing Archived re-renders exactly one column: `archived`. ### Measured, not guessed I instrumented `React.memo`'s comparator to print which props actually change identity on a collapse toggle. For every unaffected column the answer was exactly one: ``` PROBE todo changed: canDropTask PROBE in-progress changed: canDropTask PROBE in-review changed: canDropTask PROBE done changed: canDropTask PROBE archived changed: canDropTask,collapsed <- the one that should re-render ``` After: ``` PROBE archived changed: collapsed ``` ### Why this hid, and why my first attempt failed Two things worth recording, because both were mistakes I made in this program: **The test was pointed at dead code.** "keeps unaffected columns stable" measured the **legacy single-lane board**, whose props were all stable — so it passed for a long time while covering nothing operators use. Deleting that board in part 1 repointed it at the real board, where it failed 3-vs-2. I skipped it then rather than weaken it to the observed number, and said it needed its own investigation. This is that investigation. **My first fix was wrong and I was right to revert it.** In part 1 I tried a `useRef` cache invalidated by `useEffect`, it did not fix the test, and I reverted it as unproven rather than ship it. The reason is now clear: the effect runs *after* the render that populated the cache, so it wipes the very bindings that render created and the next render allocates fresh ones — the invalidation defeated the cache. `useMemo` keyed on the resolver has no such window; the map lives exactly as long as the closure owning it. ### Revert-proof The test is un-skipped **with the fix, not with a new expected number**. Restore the inline arrow at either call site and it fails 3-vs-2 again. ### Verification `pnpm test:gate` (309 + 10 + 71), `pnpm lint`, dashboard typecheck green. Board, Board.canDropTask, workflow-resolved-columns and board-no-legacy-flash: 132 passed, 0 failed, **0 skipped** — the skip introduced in part 1 is gone. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Move menus now show exactly the destinations permitted by each custom workflow, including non-adjacent moves. - Invalid or hidden destination columns are excluded from move options. - Older workflow data continues to use a compatible fallback behavior. - **Performance** - Improved board responsiveness by preventing unaffected columns and cards from re-rendering when archived sections collapse or Done sorting changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d the definitive answer on the raw flag (2 reads left, both U2b's) (#2535) ## U12 part 8 — deletes the lossy `normalizeColumn`, and ratchets it shut Independent of the #2525 → #2528 → #2530 stack; touches only `@fusion/core` exports. This closes **one of the two `@deprecated (workflowColumns, U12)` markers** the unit was named for. ### The hazard `normalizeColumn` coerced an arbitrary value to a **legacy** column, rewriting every workflow-defined custom id to `triage`. Silent data loss for any project whose workflow declares a column outside the six built-ins — and it sat one line away from `normalizeColumnId`, which sanitises structurally and passes real ids through. The dashboard picked the wrong one for its entire task-ingest path until that was diagnosed; `useTasks.ts` and `routes-trait-rekey.test.ts` still carry the notes from that fix. So this is not a hypothetical footgun — it already fired once, on the surface where it mattered most. Deleted rather than left deprecated because it has **zero callers anywhere in the workspace**. It was pure exported hazard: a lossy coercion next to its safe twin, waiting to be picked again. ### The ratchet is the point `no-lossy-column-coercion-export.test.ts` bans the **behaviour, not the identifier**: it walks every exported single-argument function whose name mentions "column" and fails if one maps a valid custom id onto a different legacy id. Re-adding `normalizeColumn` under any name trips it. Verified by actually reintroducing the function — **two of the three cases fail, including the name-agnostic one**. That last detail is what stops it being a guard that checks nothing. Coverage stated plainly: deleting an unused export has no behaviour to revert-check. The compile is the proof it had no callers; the ratchet is the proof it cannot return. --- ## Answering the standing question: does anything still read the raw `workflowColumns` flag? **Yes. Exactly two sites, and both are U2b's.** I am not able to close this out, and here is the complete list rather than a summary: ``` packages/core/src/store.ts:38,43 ← the definition packages/core/src/task-store/moves.ts:9,363 ← `useWorkflow` packages/core/src/task-store/workflow-task-create-ops.ts:11,351 ← move-policy preflight ``` That is the whole list in production code. Everything else that greps is a comment, a test that writes the flag deliberately to exercise the dead path, or the unrelated `workflowColumns.*` i18n namespace for the Columns editor panel. **Why I have not deleted the settings key.** It cannot go while those two read it — the key is what they read. And the two are not separable from each other: `workflow-task-create-ops.ts:351` computes the `movePolicyPreflight` that `moves.ts` consumes and validates, and un-gating the preflight alone would start evaluating workflow move policies (with their plugin-gate side effects) while the branch that consumes the result stays off. That is a behaviour change with no consumer, which is worse than either state. **Status of the blocker.** U2b has not landed. `main` at `919f68f9b` still has both reads; the program's merged history goes `#2466 → #2467 → #2468 (characterisation only) → #2469 → #2479 → #2500 → #2512 → #2513`, with no convergence PR. PR #2468 was Phase A2 **steps 1–2 only** — the differential characterisation — and the convergence that deletes one of the two move paths was never merged. So the honest state of the unit: everything U12 owns is done except the two reads that U2b owns, and the settings key that cannot be deleted until they are gone. If you want me to take U2b itself, say so — I have the inventory and the divergence list, and I would want the current U2b worker stood down from `moves.ts` first. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a shared hook MOVE ONLY — no behaviour change. Preparing to give ListView the same self-heal without growing a second copy of it. Board carries the FN-7591 invariant: when a rendered task's `taskWorkflowIds` mapping is absent or suspect (present, but resolving to a workflow that does not declare the task's stored column), force ONE board-workflows refetch so the real selection resolves. Signature-guarded against refetch loops, deferred one macrotask so an optimistic workflow seed lands first. ListView needs exactly this and had none of it — the gap greptile raised on #2525, which I said there I would fix separately rather than inline. Evidence the move is a move: with comments and the new wrapper signature stripped, the hook's 41 body lines and the 42 removed from Board differ by exactly one line — the `}` that closed Board's enclosing scope. Nothing else was added, removed or reordered. The original FNXC notes travel with the code, since they are the reason every line exists. Board's suite is green with no expectation edits (94 passed across Board and the cross-surface workflow-selection suite). ListView is wired to this hook in the NEXT commit, so the move and the behaviour change stay separable and independently revertable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l (PR #2525 review) Behaviour change, on top of the previous commit's verbatim extraction. A task whose `taskWorkflowIds` entry is absent — or present but resolving to a workflow that does not declare the task's stored column — gets no per-workflow move metadata, so its menu falls back to the neighbour approximation and STAYS there until some unrelated refresh happens. Board has forced one board-workflows refetch for this since FN-7591; List had none. The degraded state therefore persisted longest exactly where it is most likely: a just-created card, which is precisely when a workflow was chosen. Revert-proof: remove the hook call from ListView and the new case fails — `fetchBoardWorkflows` is never called a second time, so the mapping never resolves. A companion case pins the other half, that a fully-mapped board does NOT refetch, so the signature guard cannot turn a healthy list into a refetch loop. It measures calls made AFTER the initial load settles, because other mechanisms (mount fetch, switcher open) legitimately call the fetcher and counting from zero would measure them instead. Two existing tests needed their fixtures corrected, both because the self-heal now fires CORRECTLY where they did not expect a fetch: - "refreshes workflow columns when workflow metadata SSE arrives" chained two `mockResolvedValueOnce` payloads. The file-level cache seed maps no tasks, so first paint saw FN-001 unmapped and the repair fetch ate the payload the test asserts on. Seeded that test's own first-paint cache, and added a trailing default because the SSE swap (`backlog` -> `ready`) leaves FN-001 in a column its workflow no longer declares — a repair fetch there is right, and without a fallback it resolved `undefined` and wiped the payload. Neither was a behaviour regression; both were fixtures that had quietly depended on List never self-healing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ct Board's FN-7591 refetch and wire it up (#2530) ## U12 part 7 — the List view never self-healed a card's workflow **Stacks on #2528.** Merge that first. Paying off something I owed on #2525: greptile pointed out that a task whose `taskWorkflowIds` entry is absent — or present but resolving to a workflow that does not declare the task's stored column — gets no per-workflow move metadata, so its menu falls back to the neighbour approximation and **stays there until some unrelated refresh happens**. Board has forced one board-workflows refetch for exactly this since FN-7591. List had none. So the degraded state persisted longest precisely where it is most likely: a **just-created card**, which is when a workflow was actually chosen. I said there that porting the self-heal deserved its own change rather than riding along in a move-menu fix. This is it. ### Two commits, deliberately separable **1. Extraction — move only.** Board's ~55 lines (refs, suspect-mapping predicate, signature guard, deferred macrotask) become `useUnmappedWorkflowRefetch`. Copying them into ListView would have created a second copy of subtle race-avoidance logic to keep in sync. Evidence it is a move: with comments and the new wrapper signature stripped, the hook's **41 body lines** and the **42 removed from Board** differ by exactly one line — the `}` that closed Board's enclosing scope. Nothing added, removed or reordered. The original FNXC notes travel with the code, since they are the reason each line exists. Board's suite is green with no expectation edits. **2. Wiring — behaviour change.** ListView calls the hook. ### Revert-proof Remove the hook call from ListView and the new case fails: `fetchBoardWorkflows` is never called a second time, so the mapping never resolves. A companion case pins the other half — a fully-mapped board must **not** refetch, so the signature guard cannot turn a healthy list into a loop. It measures calls made *after* the initial load settles, because mount fetch and switcher-open legitimately call the fetcher and counting from zero would measure those instead. ### Two existing tests needed fixture corrections — neither a regression Both because the self-heal now fires **correctly** where the fixture did not expect a fetch: - `refreshes workflow columns when workflow metadata SSE arrives` chained two `mockResolvedValueOnce` payloads. The file-level cache seed maps no tasks, so first paint saw FN-001 as unmapped and the repair fetch ate the payload the test asserts on. Seeded that test's own first-paint cache, and added a trailing default — the SSE swap (`backlog` → `ready`) leaves FN-001 in a column its workflow no longer declares, so a repair fetch there is right, and without a fallback it resolved `undefined` and wiped the payload. Worth stating plainly: both fixtures had quietly depended on List *never* self-healing. That dependency is what the change removes. ### Verification `pnpm test:gate` (309 + 10 + 71), `pnpm lint`, `pnpm verify:fast` (18 steps), dashboard typecheck green. ListView + Board suites: **320 passed, 0 failed, 0 skipped**. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * List and Board views now self-recover when task-to-workflow mappings are missing or incorrect, avoiding degraded workflow UI until a later refresh. * Workflow recovery retries are more robust and coordinated to handle delayed/failed refreshes. * Recovery behavior correctly stops/reset when switching projects or unmounting. * **Tests** * Added comprehensive ListView coverage for unmapped-workflow self-heal, including retry timing, StrictMode effect replay, SSE refresh interactions, and mapped-vs-unmapped scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…st read goes — answer: 2 reads left, key cannot be deleted (#2537) ## U12 part 9 — the flag census now answers itself Independent of the #2530 rebase; adds one test file, no production changes. ## The answer, first: NO, the settings key cannot be deleted yet **Three files reference the raw flag on current main (`3ff98aae5`):** ``` packages/core/src/store.ts ← declares it packages/core/src/task-store/moves.ts:363 ← U2b: `useWorkflow` packages/core/src/task-store/workflow-task-create-ops.ts:351 ← U2b: move-policy preflight ``` Everything else that greps is a comment, a test writing the flag deliberately to reach the dead path, or the unrelated `workflowColumns.*` i18n namespace for the Columns editor panel. **Why I can't remove them.** Both are on the move path and belong to **U2b**, which carries an equivalence-proof obligation because the two move implementations it arbitrates have never both run in production. They are also **not separable from each other**: `workflow-task-create-ops.ts:351` computes the `movePolicyPreflight` that `moves.ts` consumes and validates, so un-gating it alone would start evaluating workflow move policies — with their plugin-gate side effects — while the branch consuming the result stays off. That is a behaviour change with no consumer, which is worse than either end state. **U2b has not landed.** Program history on main runs `#2466 → #2467 → #2468 → #2469 → #2479 → #2500 → #2512 → #2513 → #2525 → #2528 → #2535`. #2468 was Phase A2 **steps 1–2 only** — the differential characterisation. No convergence PR exists. ## Why this is a PR and not another status message You have asked this question three times. I have answered it three times by grepping, and each answer was a number nobody could re-derive later — including me, which is why I re-ran the audit from scratch each time. That is exactly the shape this program keeps finding: a fact everyone believes, maintained by nobody. So the census is now a test. It **fails in both directions**, deliberately: - **A new read appears** → someone re-gated behaviour on a flag that is `false` for every real project, so the feature behind it will not run. That is the defect class U12 spent its length finding (the capacity gate, the U5 guards, the move policies — all looked enforced, none were). - **The last read disappears** → U2b has landed, and the settings key can finally go. The removal steps are written at the assertion. The second case is the one that matters. It converts "remember to delete the settings key someday" into a failing test at the exact moment that becomes possible, instead of a note in a PR body that ages out. ## Verified in both directions, not assumed - Adding a reference in `lifecycle-ops.ts` → fails with `+ "packages/core/src/task-store/lifecycle-ops.ts"`. - Dropping `moves.ts` from the allowlist → fails with `+ "packages/core/src/task-store/moves.ts"`. Equality rather than subset is what makes the second case possible; a subset check would let the last reader vanish silently and leave the key orphaned forever. Two supporting assertions, both there because of failure modes this program has already hit: - **No production code WRITES the key.** That is the premise the entire unit rests on — if a writer appears, every "this branch is unreachable" conclusion in U12 needs revisiting. - **The scan sees >200 files.** A broken path glob would otherwise make every assertion vacuously green: a guard reporting success without checking anything. ## Verification `pnpm test:gate` (414 + 10 + 71), `pnpm lint`, `pnpm verify:fast`, core typecheck green. ## Standing offer If you want U12 actually closed rather than ratcheted, the remaining work is U2b's convergence. I have the inventory and the divergence list its characterisation suite does not yet cover (plugin column gates, the `transitionPending` marker, `workflowId` in `task:move` run-audit, move-policy preflight). I would want the current U2b worker stood down from `moves.ts` first — two writers on the file this whole program pivots on is the one hazard I would not take on my own authority. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added a new automated Vitest “census ratchet” to ensure only an approved, fixed set of production reads is made for the workflow columns compatibility flag. * Added checks that disallow hardcoded `workflowColumns: true/false` assignments in production sources. * Added allowlist validation, including per-file occurrence counts, required rationale text length, and confirmation that referenced files exist. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
U12 part 5 — the move menu was guessing; now it asks the graph
Stacks on #2521 (same file). Merge that first.
The context menu had no adjacency data at all, so it did two wrong things at once: it approximated move targets from a column's neighbours in declared order, and — because that approximation is strictly weaker than the real graph — it kept a
VALID_TRANSITIONSshortcut for any workflow whose column-id set matched the six built-ins.Measured, the approximation loses real operator moves:
in-progresstododoneSo every custom workflow has been offering a guess: menu entries the store would reject, and legal moves it never offered. The built-ins were fine only because the shortcut bypassed the guess entirely.
The fix
BoardWorkflowColumngainsmoveTargets, resolved byresolveAllowedColumns— the same resolvermoveTaskInternalvalidates against. The menu now offers exactly what the store will accept, for any workflow. Threaded through all four metadata builders (Board, Lane, ListView, TaskDetailModal).Optional on the wire, deliberately: a client older than this field keeps the neighbour fallback rather than losing its move menu mid-upgrade.
Why deleting the legacy shortcut is safe
Not an assertion — a measurement, then a pin.
resolveAllowedColumns(BUILTIN_CODING_WORKFLOW_IR, c)is identical toVALID_TRANSITIONS[c]for all six columns, order included:builtin-adjacency-matches-legacy-transitions.test.tspins it so the equivalence cannot drift silently — if the built-in workflow's edges change withoutVALID_TRANSITIONSfollowing, default menus change shape and that test fails first. It compares order too, since the menu renders targets in the order it receives them, so a reorder is operator-visible.Default-workflow menus are therefore byte-identical. Custom ones stop guessing.
What's left of the legacy vocabulary here
COLUMNSis gone fromTaskContextMenu— deleting the shortcut removed its last use.VALID_TRANSITIONSsurvives for exactly one thing: the no-metadata load window, documented at the site. I measured removing that in #2521 and it left Task Detail with no move options during load, which is a regression rather than a cleanup. It retires when the load window does.Revert-proof, two ways
declaredTargetsbranch → the custom-workflow case fails: the neighbour fallback returns["backlog","building"], missing the legalshippedjump and offeringbacklog, which that graph forbids. That is exactly the defect class shipped to every custom workflow today.Verification
pnpm test:gate(309 + 10 + 71),pnpm lint,pnpm verify:fast, core + dashboard typechecks green.No new test failures: five suites report 31 failures with and without the change — an identical, pre-existing set, verified by diffing failing test names against a stashed clean tree, not by comparing counts.
Summary by CodeRabbit