Skip to content

fix(chats): resolve task approvals optimistically before the API returns#67

Merged
chicoxyzzy merged 1 commit intomasterfrom
fix/agent-chat-approval-optimistic
May 9, 2026
Merged

fix(chats): resolve task approvals optimistically before the API returns#67
chicoxyzzy merged 1 commit intomasterfrom
fix/agent-chat-approval-optimistic

Conversation

@chicoxyzzy
Copy link
Copy Markdown
Member

@chicoxyzzy chicoxyzzy commented May 9, 2026

Summary

The Hecate Chat task-approval banner cleared a row only AFTER the network round-trip completed (`useRuntimeConsole.ts:1602` ➝ `await resolveTaskApprovalRequest(...)` ➝ patch). On slow links the row sat there post-click looking unresponsive, and a double-click could fire a duplicate POST through the busy-disabled buttons (since `disabled` only flipped on after the busy state propagated through React's render cycle).

Move the optimistic patch BEFORE the API call:

```
snapshot = activeAgentChatSession // sync from closure, like deleteProvider
patch the local state // banner row disappears immediately
try { await api(); maybe refresh } catch { restore from snapshot }
```

Capturing the snapshot inside the state-updater would not work: React invokes useState updaters asynchronously, and may invoke them twice under StrictMode, so a closure variable assigned inside the updater is either still null when the catch runs or already holds the patched state. Reading from the destructured `activeAgentChatSession` closure is the same synchronous-snapshot pattern `deleteProvider` uses (`useRuntimeConsole.ts:1265`).

Concurrency-safe rollback

  • The rollback uses a functional updater that bails when `current.id !== snapshot.id` — so if the operator navigated to another session while the API was in flight, the rollback no-ops instead of yanking them back.
  • It restores ONLY the specific approval activity from the snapshot (matched by `approval_id` or the projected `task:approval:` id, never by the optional `Activity.id`), so any new messages / activities that arrived during the await are preserved.

Failure handling

  • Generic failure → rollback + error notice. Operator can retry.
  • `/not pending/i` error → server says "already resolved upstream", but possibly to a different decision than the operator picked. Refresh the session to pull server-truth; if the refresh succeeds, server data wins. If the refresh ALSO fails, fall through to the same rollback so the row reflects "still pending" rather than a possibly-wrong final state, plus an error notice telling the operator to reload.

Test plan

Five new tests in `useRuntimeConsole.test.tsx` cover every code path:

  • `optimistically marks the row resolved before the API call returns` — uses a deferred Promise so the fetch is still in flight when the assertion runs; verifies `activity.status === "approved"` while `resolveResolveCall` has NOT been called yet.
  • `rolls back the optimistic patch when the API rejects` — 500 response, asserts the activity reverts to `awaiting_approval` and an error notice surfaces.
  • `rollback restores the right activity when activity.id is absent` — id-less thinking row + id-less approval row in the same message; only the approval is restored, the thinking row stays untouched.
  • `rollback does not clobber a session the operator switched to mid-flight` — operator switches a1 → a2 while the resolve hangs; API rejects; active session must stay a2 (functional updater bails on id mismatch).
  • `rolls back when the server says 'not pending' and the refresh also fails` — 409 not-pending plus a 503 on the follow-up refresh; expect `ok === false`, status restored to `awaiting_approval`, error notice.

Plus:

  • `tsc --build` — clean for production code (`useRuntimeConsole.ts`); 6 pre-existing baseline errors in unrelated test files & tsconfig.node.json that were there before this PR.
  • Full UI suite: 652 passes, 0 failures.

What's NOT in this PR

The "resolved approvals should not be clickable" half of the original ask was already covered (existing test at `ChatView.test.tsx:1167` pins the behavior — `status === "approved"` doesn't render the banner, the resolve buttons are gone). The remaining gap was the click-to-disappear latency, which this PR closes.

Copilot AI review requested due to automatic review settings May 9, 2026 17:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves Hecate Chat task-approval UX by applying an optimistic local state update so the approval banner row disappears immediately on click, then reconciling with the backend response.

Changes:

  • Apply the task-approval “resolved” patch to activeAgentChatSession before calling the resolve API, with rollback on failure.
  • Add tests covering (1) optimistic UI update before the network resolves and (2) rollback behavior when the API rejects.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ui/src/app/useRuntimeConsole.ts Moves the optimistic “resolve” patch ahead of the API call and adds rollback logic.
ui/src/app/useRuntimeConsole.test.tsx Adds two tests to pin optimistic-update timing and rollback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ui/src/app/useRuntimeConsole.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread ui/src/app/useRuntimeConsole.test.tsx
Comment thread ui/src/app/useRuntimeConsole.test.tsx
Comment thread ui/src/app/useRuntimeConsole.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread ui/src/app/useRuntimeConsole.ts Outdated
Comment thread ui/src/app/useRuntimeConsole.test.tsx Outdated
Comment thread ui/src/app/useRuntimeConsole.test.tsx
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

The Hecate Chat task-approval banner cleared a row only AFTER the
network round-trip completed: `await resolveTaskApprovalRequest(...)`
then the local patch. On slow links the row sat there post-click
looking unresponsive, and a double-click could fire a duplicate
POST through the busy-disabled buttons (since `disabled` only
flipped on after the busy state propagated through React's render
cycle).

Move the optimistic patch BEFORE the API call. Capture the
pre-resolve session synchronously from closure (same pattern
deleteProvider already uses for its rollback at line 1265) so
a genuine failure can restore the original state and let the
operator retry.

CONCURRENCY-SAFE ROLLBACK

The rollback can't just be `setActiveAgentChatSession(snapshot)` —
two real concurrency hazards:

  1. Operator may have navigated to a different session while
     the request was in flight. Replacing the active session
     would yank them back to the originating chat.
  2. A stream `session_update` or a refresh may have applied
     newer messages / activities on top of the optimistic
     state. Wholesale replacement drops them.

Both are addressed by a functional updater that:
  • bails when `current.id !== snapshot.id` — operator
    navigated away, leave them be.
  • restores ONLY the specific approval activity from the
    snapshot, leaving every other field untouched. Stream
    updates and refresh-driven additions survive.

The rollback predicate matches the activity by `approval_id`
(or the projected `task:approval:<id>` id), NOT by the optional
`Activity.id` — matching by id alone could (a) fail to restore
when the current row has no id and (b) wrongly match the first
id-less row in the snapshot if both happen to be undefined.

FAILURE HANDLING

The "/not pending/i" error branch was dishonest about reality.
Server returning "approval already resolved" does NOT guarantee
the resolution matches the operator's chosen decision — another
tab might have approved while this one tried to reject, the run
might have timed out into auto-rejection, or the run might have
been cancelled. The previous code refreshed the session to pull
server-truth, but on refresh failure (network blip, gateway
transient) the optimistic patch stayed on screen, potentially
showing a decision the server never made.

Restructure: refresh first; on success return true (server data
wins). On refresh failure, fall through to the same surgical
rollback the generic catch path uses, plus an error notice
telling the operator to reload. The row reflects "still pending"
rather than a silently-wrong final state.

Extracted the rollback into a closure-scoped
`rollbackOptimisticApproval` helper so both the
generic-failure path AND the not-pending+refresh-failed path
share the same behavior structurally rather than by discipline.

WHY CAPTURING SNAPSHOT INSIDE THE STATE-UPDATER WOULD NOT WORK

React invokes useState updaters asynchronously, and may invoke
them twice under StrictMode. A closure variable assigned inside
the updater is either still null when the catch runs or already
holds the patched state. The `deleteProvider` synchronous-from-
closure pattern is the right shape.

TESTS

Five tests in useRuntimeConsole.test.tsx cover every code path:

  - optimistically marks the row resolved before the API call
    returns (deferred Promise; assert state mutated while fetch
    is still in flight)
  - rolls back the optimistic patch when the API rejects (500)
  - rollback restores the right activity when activity.id is
    absent (id-less thinking row + id-less approval row in same
    message; only approval restored)
  - rollback does not clobber a session the operator switched
    to mid-flight (navigate a1 → a2 while resolve hangs; API
    rejects; active session must stay a2)
  - rolls back when the server says 'not pending' and the
    refresh also fails (409 + 503 on follow-up refresh)

UI suite clean: 652 passes.

WHAT'S NOT IN THIS PR

The "resolved approvals should not be clickable" half of the
original ask was already covered (existing test at
ChatView.test.tsx:1167 pins the behavior — `status === "approved"`
doesn't render the banner, the resolve buttons are gone). The
remaining gap was the click-to-disappear latency, which this PR
closes.
@chicoxyzzy chicoxyzzy force-pushed the fix/agent-chat-approval-optimistic branch from 6753042 to 9ede401 Compare May 9, 2026 19:23
@chicoxyzzy chicoxyzzy merged commit 0f87b00 into master May 9, 2026
10 checks passed
@chicoxyzzy chicoxyzzy deleted the fix/agent-chat-approval-optimistic branch May 9, 2026 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants