fix(web): stop a task status from reverting when an older update finishes late

[nova28]

Tip

PR walkthrough: Open the visual walkthrough

Today: Dragging a task on the Office board (or editing its status/priority/assignee through a
property picker) twice in quick succession can leave the UI showing a stale value: if an older
request fails after a newer one for the same task already succeeded, the older failure's rollback
overwrites the newer, server-confirmed state — and it stays wrong until an unrelated refetch or a
page reload.

After this: A failed mutation only rolls back its optimistic patch when it is still the last
word for that task. Once a newer mutation has settled successfully, a late failure surfaces its
error toast but no longer clobbers the board.

Who hits this: Anyone re-triaging tasks quickly on the Office kanban board, or clicking through
property pickers (status, priority, assignee, project, parent, blockers, reviewers, approvers)
faster than the network round-trip.

Scope: standalone fix, zero backend, 9 files under apps/web/.

Not here: Interference between two different properties changed by two different in-flight
mutations on the same task (e.g. priority fails after status succeeds) is not separately guarded —
the whole-task write-guard scope this PR adds relies on the existing task.updated WebSocket
broadcast + refetch to reconcile that case, which was verified safe but is undocumented and
untested. That's tracked as a follow-up rather than folded into this PR.

Important Changes

• Extended the existing per-field write-guard in lib/state/office-task-content-sync.ts with a
  whole-task TASK_SCOPE, so useOptimisticTaskMutation and the board's applyStatusDrop both
  compare a monotonic per-task sequence before restoring a snapshot on failure, instead of
  restoring unconditionally.
• Added recordWriteSettled: the approval-gate redirect path now records its server-persisted
  status as settled before deciding whether to roll back, so a gate redirect isn't lost even when
  a newer mutation is still in flight.

Validation

• make typecheck — clean
• make lint-backend (0 issues) / make lint-web (eslint --max-warnings 0, clean) / make lint-format — clean
• pnpm run i18n:ratchet (from apps/web) — clean, 0 added + 4 modified files
• vitest — 6 files, 99 tests passed (use-board-drag.test.ts, status-picker.test.tsx,
  use-optimistic-task-mutation-rollback-ordering.test.tsx, office-task-content-sync.test.ts,
  use-optimistic-task-mutation.test.tsx, office-tasks.test.ts)
• E2E (chromium, host runner): tests/office/property-pickers.spec.ts — 13 passed;
  tests/office/tasks.spec.ts — 12 passed
• Re-ran the full gauntlet after rebasing onto current main; all of the above stayed green

Three unrelated failures on this runner, none touched by this diff:

• internal/worktree (Go): identical failure set reproduced against the merge-base commit in a
  scratch worktree — a temp-directory/filesystem quirk on this machine, not a regression.
• apps/web/lib/http-git-server.test.ts: 3 failures require a running Docker daemon; this runner
  intentionally does not run Docker.
• make lint-harness / lint-specs: this runner's Python is 3.9.6, which doesn't support the
  str | None syntax those scripts use (needs 3.10+); the scripts themselves are unchanged by this
  diff.

Possible Improvements

Low risk: the whole-task write-guard is coarser than per-field, but the coarser scope was verified
safe against the WS reconciliation path rather than assumed; a follow-up card tightens the scope and
adds the missing test/doc coverage for that invariant.

Checklist

• If I do not have repository write access and this is a large architectural change, I discussed the direction in a linked issue before opening this PR.
• This PR contains one logical change; unrelated work is split into separate PRs.
• I have performed a self-review of my code.
• I have manually tested my changes and they work as expected.
• My changes have tests that cover the new functionality and edge cases.
• If my change touches UI files (apps/web/), I have added or updated Playwright e2e tests in apps/web/e2e/ and verified them with make test-e2e.
• I checked whether this affects public docs in docs/public/** and updated them or noted why no docs change is needed.

Preview Environment

URL	https://kandev-pr-3528-bwo7.sprites.app
Commit	ab90472
Agent	Mock agent

Updates automatically on each push. Destroyed when the PR is closed.

[coderabbitai]

Warning

Review limit reached

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: c31c5cdf-59d9-4112-9d83-a4cb45ccda35

📥 Commits

Reviewing files that changed from the base of the PR and between 401947f and ab90472.

📒 Files selected for processing (9)

• apps/web/app/office/tasks/use-board-drag.test.ts
• apps/web/app/office/tasks/use-board-drag.ts
• apps/web/components/task/simple/components/status-picker.test.tsx
• apps/web/components/task/simple/components/status-picker.tsx
• apps/web/e2e/tests/office/property-pickers.spec.ts
• apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx
• apps/web/hooks/use-optimistic-task-mutation.ts
• apps/web/lib/state/office-task-content-sync.test.ts
• apps/web/lib/state/office-task-content-sync.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Claude finished @nova28's task in 5m 10s —— View job

---

Code Review

Tasks

• Load review skill and gather context
• Read changed files
• Analyze correctness of the write-guard logic
• Check test coverage and edge cases
• Review E2E tests
• Post findings

---

Findings

Suggestion (recommended, doesn't block)

1. ctx.restore(snapshot) includes title/description despite the comment saying it must not — store and local state briefly diverge — apps/web/hooks/use-optimistic-task-mutation.ts:119

The comment at lines 111–118 correctly explains that the rollback must not touch title/description, because those fields are managed by the per-field guard. The store rollback on line 121 correctly honours this by destructuring them out:

const { title: _title, description: _description, ...storeRollback } = storeSnapshot;
storeApi.getState().patchTaskInStore(taskId, storeRollback);

But the very next line for the local task state does not apply the same exclusion:

ctx.restore(snapshot);  // ← restores Task including .title and .description

snapshot = ctx.task is captured at mutation-issue time. In the common case — no title/description write in flight — this is harmless. But if useCommitTaskTitle applied its optimistic patch (ctx.applyPatch({ title: "New" })) after snapshot was captured and before this status/priority mutation failed, ctx.restore(snapshot) sets the local task's title back to "Old", while the store correctly keeps "New". The two representations diverge until the next WS refetch corrects the local state.

The PR description acknowledges this as an out-of-scope follow-up ("verified safe but undocumented and untested"). A minimal fix that preserves the current contract without changing ctx.restore's type:

// Only restore fields this hook actually patched — leave title/description
// to their own per-field guard writers (useCommitTaskTitle / useCommitTaskDescription).
const { title: _t, description: _d, ...safeSnapshot } = snapshot as Task & {
  title?: string;
  description?: string;
};
ctx.restore(safeSnapshot as Task);

Or, if Task is known never to have a description key on the local task object when accessed through this hook's callers, a comment clarifying that invariant would at least close the documentation gap.

---

2. Comment at lines 111–118 is accurate for the store but inaccurate for the local state — apps/web/hooks/use-optimistic-task-mutation.ts:111

The comment says "the rollback must not touch [title/description] either", but the local-state restore immediately below does touch them. The comment trains the next reader to think the invariant holds at both layers, which it currently does not. Even if (1) is deferred, the comment should be scoped: "the store rollback must not touch them either" — and ideally note that the local state is reconciled by the WS refetch.

---

Everything else looks correct and well-guarded:

• shouldRestoreAfterFailedWrite is called before endWrite in both the hook and applyStatusDrop — correct: the current sequence must still be visible in pendingWrites when checking for later-sequenced pending writes.
• recordWriteSettled for ApprovalGateError is called before shouldRestoreAfterFailedWrite. Since the check is lastSuccess > sequence (strict), settling on one's own sequence does not suppress the same write's own redirect restore. ✓
• TASK_SCOPE and field scopes use fully independent guard maps; the new test at line 456 ("keeps the 'task' scope and a field scope independent") pins this. ✓
• beginWrite / endWrite / recordWriteSettled ordering in the success path is safe — no await between them, so no concurrent failure can interleave. ✓
• The E2E test uses injectLatency (the sanctioned form) and waitForHttp causal waits rather than page.waitForTimeout. ✓
• The chained double-failure test in use-optimistic-task-mutation-rollback-ordering.test.tsx covers both resolution orders for the newer-fails-first and older-fails-first cases. ✓
• applyStatusDrop restores the whole OfficeTask snapshot (including rawStatus) on plain failure, which is intentional to prevent re-normalization to the wrong column. ✓

---

Summary

Severity	Count
Blocker	0
Suggestion	2 (same root cause)

Verdict: Ready with suggestions — the generation guard and its tests are correct; the only open issue is the acknowledged local-state/store divergence on the rollback path, which the PR itself flags as a follow-up.

[greptile-apps]

Greptile Summary

This PR introduces a per-task mutation sequence guard for Office task updates, including board drags, property pickers, and approval-gate redirects.

• Prevents an older failed mutation from immediately overwriting a newer successful mutation.
• Moves approval-gate redirect settlement into the shared optimistic-mutation path.
• Adds unit, component, and Playwright coverage for several overlapping-request orders.
• The guard does not yet converge correctly when the older request fails first and the newer request subsequently fails.

Confidence Score: 4/5

This PR is not safe to merge until overlapping mutations converge when the older request fails before a newer request that also fails, and the explicit production-comment rule is satisfied.

Suppressing the older failure permanently loses its rollback, allowing the newer failure to restore a snapshot containing an unpersisted optimistic value; the same mechanism can overwrite a persisted approval-gate redirect.

Files Needing Attention: apps/web/lib/state/office-task-content-sync.ts, apps/web/hooks/use-optimistic-task-mutation.ts, apps/web/app/office/tasks/use-board-drag.ts

Important Files Changed

Filename	Overview
apps/web/lib/state/office-task-content-sync.ts	Widens write guards to a whole-task scope, but permanently discards suppressed failure recovery needed when all overlapping writes fail.
apps/web/hooks/use-optimistic-task-mutation.ts	Applies task-scoped sequencing and approval redirects to generic mutations, while retaining issue-time snapshot rollback semantics that expose the double-failure ordering bug.
apps/web/app/office/tasks/use-board-drag.ts	Adds the same task-scoped guard to status drops and therefore shares the unresolved rollback-ordering failure.
apps/web/components/task/simple/components/status-picker.tsx	Removes component-specific approval redirect recovery in favor of the shared mutation hook.
apps/web/hooks/use-optimistic-task-mutation-rollback-ordering.test.tsx	Adds substantial overlap coverage, but omits the older-fails-first/newer-fails-later ordering.
apps/web/app/office/tasks/use-board-drag.test.ts	Covers late stale failures and approval redirects but not sequential failure in the opposite completion order.
apps/web/e2e/tests/office/property-pickers.spec.ts	Adds realistic browser coverage for an older failure arriving after a newer successful status update.

Sequence Diagram

sequenceDiagram
    participant UI
    participant Guard
    participant A as Older request A
    participant B as Newer request B
    UI->>Guard: Begin A and apply optimistic value A
    UI->>Guard: Begin B and snapshot value A
    A-->>UI: Failure
    UI->>Guard: Restore A?
    Guard-->>UI: No, B is pending
    UI->>Guard: End A and discard its rollback
    B-->>UI: Failure
    UI->>Guard: Restore B?
    Guard-->>UI: Yes
    UI->>UI: Restore B snapshot containing value A
    Note over UI: Both requests failed, but value A remains visible

Loading

_{Reviews (1): Last reviewed commit: "fix: record settlement for approval-gate..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab90472ac4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[nova28]

Checked — this doesn't reproduce. ctx.restore is restoreTask, defined in apps/web/app/office/tasks/[id]/page.tsx:408-417:

const restoreTask = useCallback(
  (snapshot: Task) => {
    setTask((prev) =>
      prev && prev.id === id
        ? { ...snapshot, title: prev.title, description: prev.description }
        : prev,
    );
  },
  [id, setTask],
);

It spreads snapshot but then unconditionally overrides title/description with the current prev.title/prev.description. So no matter what title/description snapshot carries — stale or otherwise — they're discarded before the state update commits. ctx.restore(snapshot) at use-optimistic-task-mutation.ts:119 already excludes title/description at the local-state layer, the same invariant the store rollback enforces two lines below via destructuring. The comment at :110-118 is accurate for both layers, not just the store. No code change needed here.