fix(dashboard): an operator's typed task title was wiped when board workflows resolved - #3286
Conversation
…orkflows resolved The Research task modal reset `title`, `description`, `priority` and `taskId` in the same effect that fetched the task list, and that effect's dependency list carried `isArchivedColumn` for the fetch's sake. `isArchivedColumn` is a useMemo over `boardWorkflows` from `useBoardWorkflows()`, so its identity changes every time that hook resolves or revalidates — and each change re-ran the reset over whatever the operator had already typed. Symptom: open Create Task from a research finding, type a title before the board workflows settle, and the field silently reverts to the derived default `Research: <heading>`. The task is then created with a title the operator did not write. Introduced by #3215, which correctly added the archived-column filter but attached its dependency to an effect that also owns form state. Same class as docs/solutions/ui-bugs/skill-autocomplete-highlight-reset-on-swr-revalidation.md. Split into two effects: the reset depends only on what it derives from, the fetch keeps `isArchivedColumn`. No behaviour change to the archived filter itself. Verified both directions on the three standing tests, which fail without this and pass with it: with isArchivedColumn back on the reset effect: 3 failed | 24 passed (27) as committed: 27 passed (27)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe modal separates form resets from enrich-mode task loading. Form values reset only when modal or source finding inputs change. Task loading clears stale results, ignores superseded requests, and excludes tasks in archived columns. ChangesResearch task modal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
Correcting my own verification numbers, and the result is stronger than what I first claimed. My original figures ( 1. One failure I attributed to this bug was never real. 2. The remaining failures are a race, which is why the count moved. On That non-determinism is not noise — it is the bug's signature. The defect is a race between the operator typing and 3. The fix removes the race rather than shifting it. Four consecutive runs on this branch: Splitting the effect makes the reset independent of when the async hook settles, so there is no window left for the resolution to land inside. A timing fix that merely narrowed the window would still show occasional failures across four runs; this does not. Corrected summary for review:
The product diagnosis and the fix are unchanged — the effect dependency was and is the cause. What changed is that I can now state the evidence accurately: it is a race, it is not deterministic, and one of the failures I bundled with it belonged to a stale dependency instead. Method note, since it is the same lesson twice tonight: I measured a "red main" against an environment that did not match the lockfile, and reported it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ResearchTaskActionModal.tsx`:
- Around line 91-112: Update the task-fetching useEffect around fetchTasks to
invalidate prior requests during cleanup and clear tasks before starting each
new request. Track whether the current effect instance is still active, and only
apply filtered results or loading-state updates while it remains current; ensure
cleanup prevents superseded requests from overwriting tasks after open, mode,
projectId, or isArchivedColumn changes.
🪄 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: 8247a1f7-ad8e-444a-91aa-16929abbc151
📒 Files selected for processing (1)
packages/dashboard/app/components/ResearchTaskActionModal.tsx
#3286 review. The enrich-mode effect refetches when projectId or isArchivedColumn changes, with no guard, so a slower earlier request can resolve last and repopulate the picker from the previous project. Worse, the old rows stayed listed while the new fetch was in flight, so an operator could select a task from a project they had already switched away from and attach a finding to it. Wrong-row attachment, not flicker. Adds a per-effect superseded flag cleared in cleanup, and empties the list on entry so there is no stale-but-selectable window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Real, and fixed. The failure is wrong-row attachment, not flickerThe enrich-mode effect refetches when That is the same class as the bug this PR already fixes ( Fix
The second half matters as much as the first: without it the guard prevents the stale overwrite but leaves the stale selection window open, which is the half that loses data. What I did NOT doNo regression test. Driving this needs two overlapping in-flight fetches with controlled resolution order against a modal that owns its own effect — the existing Flagging rather than skipping quietly: this is a real coverage gap on a data-correctness fix, and if you want it held, the honest shape is a deferred-promise fixture resolving request #1 after request #2. Verification
|
|
This regression is mine — #3215 added that dependency — and your diagnosis is exactly right. I added Verified your fix keeps the coverage #3215 addedon Why I did not catch itTwo reasons, both worth recording since this repo already documents the class:
The generalisable rule your note states well: an effect that both fetches and owns form state has two dependency lists, and satisfying one corrupts the other. Splitting is the only fix that does not trade one bug for the other. Nothing to change from me — flagging authorship so the history is not ambiguous, and because #3264's "dashboard red on main" was a real product defect rather than a stale test, which is the less comfortable of the two readings. |
…caused (#3290) #3286 fixed a real user-facing regression and **shipped no test**, so nothing stops it returning. The regression was mine. ## The bug #3215 (mine) added `isArchivedColumn` to an effect's dependency list to keep the task fetch honest. That effect **also owned four `setState` calls**, and `isArchivedColumn` is a `useMemo` over `useBoardWorkflows()` — which revalidates asynchronously. Every revalidation re-ran the reset over whatever the operator had typed. A title entered before the workflows settled silently reverted to `Research: <heading>`, and the task was created with a title nobody wrote. ## Why my own four tests could not see it Every existing case in this file asserts the **filtered task list** — render, await `fetchTasks`, read the datalist. **None types into the form.** I tested what I added and not what I touched. That is why the regression belongs in this file rather than a new one: the gap is this file's. ## The case Renders with `boardWorkflows: null` — the state when an operator opens the modal and starts typing — types a title with per-character `userEvent`, then rerenders with a resolved workflow set (a **new object identity**, which is the entire mechanism) and asserts the typed text survived. Two details that each cost a cycle, recorded at the site: - **`fetchTasks` is not awaited.** It runs only in enrich mode, while the title field exists only in create mode — so the reset effect, not the fetch, is under test. My first version waited on it and failed for the wrong reason. - **`userEvent.type`, not `fireEvent.change`.** The documented failure is state overwritten between renders; a single synthetic change event can land after the reset and mask it. ## Measured both directions, on main `6834ba35bd` | state | result | |---|---| | fixed main | **5 passed** | | dependency re-added to the reset effect (my bug) | **1 failed / 4 passed** — and only that case | The second row is the point: it fails on precisely the mutation that recreates the defect, and leaves the four archived-lane cases green — so it pins the regression without duplicating what is already covered. `eslint` clean, `check-fnxc-future-dates` 0. Test-only. ## Note on provenance Getting this measurement took three attempts: a `git checkout` of the PR branch silently failed (stderr suppressed), so I twice ran against the wrong tree and nearly concluded the fix did not work. HEAD and dirty-count are printed beside every number above for that reason.
…you report (#3291) Extends the doc from #3255/#3273 with the failure that cost the most in a single session: **one stale install produced five wrong reports on one issue** (#3264). ## What happened A `node_modules` that had drifted from the lockfile — `jsdom@29.0.1` installed, `29.1.1` pinned — generated failures that existed on no CI machine and no other checkout. They were not subtle: deterministic, reproducible on demand, with plausible stack traces and real-looking assertion diffs. Each round of triage got **more precise about the wrong data**: | round | claim | why it was wrong | | --- | --- | --- | | 1 | "4 deterministic failures" | measured in a 4-file batch, called it isolation | | 2 | "3 deterministic, 2 order-dependent" | isolated correctly, but a race is not deterministic | | 3 | "TaskCard is broken" | stale jsdom; the CSS assertion was correct | | 4 | "no contamination" | true of four app files; published unqualified | | 5 | "quarantine these two" | never read the failure text — both were timeouts | The through-line is not carelessness about the code. **The environment was never treated as part of the claim**, so no amount of care about the analysis could recover it. ## The checks, in the order they cost the most ```bash pnpm install --frozen-lockfile # node_modules is not evidence until it matches the lockfile <run the file ALONE, 3+ times> # isolation and repetition answer different questions <read the failure TEXT> # a timeout and an assertion failure need opposite responses uptime # a loaded box manufactures timeouts that mean nothing ``` ## Why the load check earned its place Two tests "failing" in a full-suite run were `Test timed out in 15000ms` on a box at **load average 9.7 with 84 users**. Under AGENTS.md's quarantine-on-sight rule that reads as a flake to quarantine — and the ledger's **14-day deletion ratchet would have made the lost coverage permanent**. The rule presumes the failure is a property of the test, not of the machine. A wall-clock budget crossed under local contention is evidence about the hardware. I was one comment away from deleting healthy coverage on that basis. ## The tell A finding is environment-derived when it is **local, recent, and unshared**: nobody else has reported it, CI is green, and it appeared without a commit that could explain it. Any two of those should stop a report before it is written. All three applied here, and the report went out anyway — five times. ## Verification Docs only; no code paths change. `fnxc-future-dates`, `lifecycle-columns`, `quarantine-ledger` exit 0. No changeset — internal docs are excluded. Context: the one finding in #3264 that survived all five rounds is #3286 (merged), and it survived because it was verified by **reverting the product change** rather than by trusting a red — 3/3/2 failures without the fix, 27/27 across four runs with it.
Found by chasing the deterministic half of #3264 (dashboard red on
main). The tests were right; the product is broken.The bug
Open Create Task from a research finding, type a title before the board workflows settle, and the field silently reverts to the derived default
Research: <heading>. The task is then created with a title the operator did not write.description,priorityandtaskIdreset the same way.ResearchTaskActionModalreset those four fields in the same effect that fetched the task list, and that effect's dependency list carriedisArchivedColumn:useBoardWorkflowsresolves and revalidates asynchronously, so the memo's identity changes and the reset re-runs over whatever the operator has typed.Introduced by #3215, which correctly added the archived-column filter but hung its dependency on an effect that also owns form state. Same class as the documented
docs/solutions/ui-bugs/skill-autocomplete-highlight-reset-on-swr-revalidation.md.The fix
Split into two effects: the reset depends only on what it derives from; the fetch keeps
isArchivedColumn. No behaviour change to the archived filter — #3215's guard is untouched.Verification, both directions
The three standing
ResearchViewtests fail without this and pass with it:What I tried and removed, because it matters
I wrote a dedicated invariant test (per "fix the invariant, not the repro") asserting that all typed fields survive a revalidation. I deleted it, because it did not work.
mockImplementationOnceto deferfetchBoardWorkflows.ResearchViewresolves board workflows on mount, so that once-implementation was consumed before the modal opened. Reverting the product fix left the test green — it proved nothing.beforeEachusesvi.clearAllMocks(), which clears calls but not implementations, so the deferral leaked into later tests and leftfetchBoardWorkflowspermanently pending — masking two of the three genuine failures. The revert then showed1 failedinstead of3, i.e. my test was hiding real bugs.Rather than ship a regression test that cannot regress, I removed it. The three existing tests already fail without the fix, which is real coverage; a broader invariant test needs a modal-level harness that resets implementations between cases, and that is worth doing properly rather than badly here.
Scope
Also in #3264:
TaskCard.badge-wrap(1 deterministic failure, unrelated — CSS/layout), anduseChat/WorkflowNodeEditor/PlanningModeModal, which pass standalone and are cross-file contamination, not product bugs. Untouched here; the issue has the per-file matrix.No changeset —
@fusion/dashboardis private.Summary by CodeRabbit