Repository navigation
feat(web): queue and approvals sit above the composer again - #246
Conversation
The 2026-10-28 sync moved queued messages into the timeline as dashed bubbles without reordering, and moved approvals onto their timeline row. Queued messages are the collapsible "Queued" banner above the composer again, with drag and arrow-key reordering; the server still accepts queued-run.reorder, so only the client command comes back. A pending approval is a notice card in the composer banner stack, styled like the fork's other notices, and the timeline keeps plain rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code quality review of 0350f128ae9295b9b660279b6ac48f65cef100a3. Not approving. Two of the findings below fail the approval bar.
1. The empty approvals list switches off the timeline's "waiting on the user" state (blocker)
pendingApprovals={FORK_NO_TIMELINE_APPROVALS} (ChatView.tsx:3695) treats pendingApprovals as if it only decided whether the inline row renders. It decides more than that. The row builder reads the same set to decide whether the agent is blocked:
MessagesTimeline.logic.ts:2103: the Thinking row is suppressed only whilependingApprovalRequestIds.size > 0.MessagesTimeline.logic.ts:1541:latestToolKeepsActivityLivestays off while an approval waits.TimelineWorkGroupRows.tsx:378:ThinkingTimelineRow'slive/shimmerisctx.pendingApprovals.size === 0.MessagesTimeline.logic.ts:1479: un-collapses the pending approval entry.
With an empty list, a run blocked on approval (phase === "running", so isWorking) looks to the timeline like a run that is still working. That brings back the live/shimmering activity row while nothing is running: a spinner that lies, and a constant repaint. I traced this through the code and did not reproduce it at runtime. A quick check with a seeded command approval should confirm it.
It also leaves dead wiring behind. heldApprovalRequestIds, respondingRequestIds and onRespondToApproval still go into ThreadTimeline, and ChatComposer's held-ids layout effect (ChatComposer.tsx:674) now feeds a list that is always empty.
Simpler fix: the read-only state already exists. onRespondToApproval is optional on MessagesTimeline, and TimelinePendingApprovalRow already renders "Waiting for approval" without buttons when no responder is passed ({onRespondToApproval ? (…actions) : null}). So keep pendingApprovals={pendingApprovals} and fence a single change that leaves onRespondToApproval off <ThreadTimeline>. That:
- deletes
FORK_NO_TIMELINE_APPROVALS - keeps every waiting/live rule correct
- keeps the held-ids channel meaningful
- leaves a truthful "waiting" marker in the timeline, while the answer lives only above the composer.
If you really want no marker in the timeline, shadow the 122-line leaf TimelinePendingApprovalRow under overrides/ (fork rule #2). Don't fake the data the row model depends on.
2. ForkQueuedRunsBanner copies upstream's queue model instead of sharing it (blocker)
Lines 45–235 of custom/ForkQueuedRunsBanner.tsx are a near-verbatim copy of QueuedRunsControl's non-visual half:
queuedImageResources- the acknowledged-message filter
- the server and optimistic item derivation
sendQueuedMessagewithsteerInFlightRef- the
steerNext/editLatestimperative handle remove
The PR description lists three upstream behaviors that had to be ported by hand: Send now on a held queue, guests can't edit, and edit is blocked by an open question. That list is the problem. The next upstream change to queue semantics will not conflict. It will drift silently. The guard tests only pin strings, and the watch: entry only tells someone to go look.
ChatView's hasQueuedTurns (ChatView.tsx:3234) is a third copy of the "optimistic message not yet acknowledged" rule. It now only gates a component that already returns null when it has no items.
Remedy: put the model in one place. One fenced extraction in QueuedRunsControl.tsx would do it: useQueuedRunsModel(props, ref) returns { items, workflow, canSend, busyRunId, send, remove, … }, and both upstream's component and the fork banner render from it. The fork banner then only owns presentation and reordering. That is a slightly larger upstream fence, but it turns silent drift into a loud merge conflict, which is the right trade for this much logic.
3. The row model spreads a nullable mode across the whole render (spaghetti)
items gives every row runId: RunId | null, messageId: … | null, serverIndex: number | null and pending: boolean. All four encode one fact: whether the server has the row yet. As a result the 300-line items.map body re-checks it about ten times:
item.runId === null ||in the Edit, Steer and Removedisabledpropsif (item.runId !== null …)in threeonClicksrowRunId !== null && rowServerIndex !== nulltwiceitem.serverIndex === nullinonDragOveritem.runId === nullinonDragStart
It also mixes rowRunId aliases with item.runId in the same scope.
A pending row has no grip and no enabled actions; it only shows a clock. Render pending rows with their own small SavingQueuedRow, and give the server rows a non-null { run, serverIndex, … } shape. Every one of those guards then disappears. While you're there:
- pull the row into a
QueuedRunRowcomponent - collapse the three
setBusyRunId(id); await …; setBusyRunId(null)copies (move,remove,sendQueuedMessage) into onewithBusy(runId, command)
The nested template-ternary tooltip strings (Steer and Edit) also want a named helper.
4. Smaller boundary points (approval notice)
ChatComposerProps.onRespondToApprovalis typed asParameters<typeof forkApprovalNoticeItem>[0]["onRespondToApproval"](ChatComposer.tsx:450). That is a two-hop derived type, and it ties an upstream component's public props to a fork builder's parameter object. Export a named responder type fromforkApprovalNotice.tsx, or reuse the timeline'sTimelineRowSharedState["onRespondToApproval"].APPROVAL_LABELandDETAIL_LABELduplicate the label strings inTimelinePendingApprovalRow'sAPPROVAL_PRESENTATION. If you take the override route in #1, export one table from there.- In the notice, the
canRespond === falsemessage ("Provider process is gone…") renders inside a<code aria-label="Command">. Screen readers announce it as the command. - The light-scheme CSS (
theme.custom.css:3730) uses:has([data-fork-approval-notice])to reach from a descendant marker back into an upstream slot and patch one consumer of a dark-only notice layout. If notice cards should stack in light, scope that to[data-fork-composer-notice]once rather than special-casing approvals.
File size
ChatView.tsx (4304→4320) and ChatComposer.tsx (3942→3974) were already well past 1k lines. No file crosses the threshold here, and the inline hunks are small and fenced, which is fine.
Summary: #1 is a behavior regression hidden behind an "empty data" trick, and the existing optional responder deletes it with less code. #2 and #3 are the structural debt in the new banner. #1 and #2 need resolving before this meets the bar.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Reviewed commit 0350f12 against base 06fe8eb. Two findings are attached inline.
Validation: 45 tests passed across the two new fork guard files, threadWorkflows.test.ts, threadRequests.test.ts, and userMessage.test.ts. The findings come from tracing the changed rendering paths and their existing consumers; these passing tests do not exercise the reported UI regressions. No live-client verification was performed. No changes were pushed.
| forkApprovalNoticeItem({ | ||
| approval: activePendingApproval, | ||
| pendingCount: pendingApprovals.length, |
There was a problem hiding this comment.
[P2] Keep the displayed approval set aligned with the send context
With two simultaneous approvals that do not support feedback, this renders only the first request (as 1/2), and the timeline no longer renders either approval control. However, getSendContext() still puts every unheld approval into shownPendingApprovalIds (lines 3116–3118), and useComposerSend uses that list to decline every matching request before redirecting the agent. Sending a message while reviewing the first notice therefore also declines requests that were never presented in that notice. Render the approvals included in that list, or update the send interaction so the single-request presentation does not silently act on the remaining requests.
There was a problem hiding this comment.
Same fix as the thread above (23c6861): the send context now carries only the approval the notice shows.
| ...queued.map(({ run, text, attachments, context }, serverIndex) => ({ | ||
| key: run.id, | ||
| runId: run.id, | ||
| messageId: run.userMessageId, | ||
| serverIndex, | ||
| text, | ||
| // A guest's message keeps the guest's words; the owner can cancel it, not rewrite it. | ||
| editable: context?.guest === undefined, |
There was a problem hiding this comment.
[P2] Preserve sender attribution in the compact queue rows
The replaced QueuedRunsControl uses the queued message with resolveUserMessagePresentation to show guest names and identify agent/automation submissions. This mapping drops message and uses context.guest only to disable editing, so the rendered banner shows all senders as identical plain text. In a shared thread, a guest's queued instruction now looks like the owner's own message; queued agent and scheduled-task instructions likewise lose their source. Carry the attribution into the new row and retain a compact sender label so moving the queue does not remove the information needed to decide which message to steer or remove.
There was a problem hiding this comment.
Fixed in 23c6861. Rows keep a compact sender label from resolveUserMessagePresentation: the guest's name (guest colour, with "(guest)" for screen readers), or Agent, Automation, or T3 Code. The owner's own messages stay unlabeled.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review of 0350f128ae9295b9b660279b6ac48f65cef100a3.
I found two issues, both in the approval half. The queue banner looks right: queued-run.reorder exists in contracts and the orchestrator with the same runId/beforeRunId shape, the arrow-key beforeRunId math is correct, and hasQueuedTurns gates the banner the same way it gated the old timeline footer.
- (High) While an approval is pending, the timeline animates the approval row as live work. Passing an empty
pendingApprovalslist also clears the pending set that the timeline uses to know the agent is blocked, not just the inline approval row. - (Medium) Sending a message declines approvals the user can't see. The notice shows only the first approval, but a send still declines every unheld approval.
Details are in the inline comments.
🤖 Generated with Claude Code
| pendingApprovals={pendingApprovals} | ||
| /* fork:begin fork-composer-approval-notice — see .fork/customizations.yaml#fork-composer-approval-notice | ||
| Approvals answer from the notice above the composer, so the timeline keeps plain rows. */ | ||
| pendingApprovals={FORK_NO_TIMELINE_APPROVALS} |
There was a problem hiding this comment.
While an approval is pending, the timeline animates the approval row as live work.
MessagesTimeline turns pendingApprovals into pendingApprovalRequestIds, and deriveMessagesTimelineRows relies on that set for more than drawing the inline row:
isPendingApprovalkeeps the approval out ofactiveToolEntries.latestToolKeepsActivityLiveand thethinkingrow condition both check!pendingApprovalRequestIds?.size.ThinkingTimelineRowsetslive = ctx.pendingApprovals.size === 0.
With FORK_NO_TIMELINE_APPROVALS, all of these act as if nothing is waiting. I checked with a throwaway probe: a command row plus a waiting approval_request, with isWorking: true:
pending=1: ["working","work","work"]
pending=0: ["working","work-live"] <- this PR
So while the agent is blocked on the approval, the transcript shows the approval as a shimmering live-activity row. That's a misleading spinner, and it's a continuous animation for as long as the approval sits there. The existing test groups approval and user-input requests with commands … when waiting (MessagesTimeline.logic.test.ts:244-274) pins this upstream behavior, including approvalOnlyRows.some(row => row.kind === "thinking") === false.
Suggested fix: keep passing the real pendingApprovals so row derivation and liveness stay correct, and suppress only the inline control. For example, a fenced check where TimelinePendingApprovalRow is chosen (TimelineWorkGroupRows.tsx:512 and TimelineWorkEntryRow.tsx:204) could fall through to the plain row. The unmatchedPendingApprovals block in MessagesTimeline.tsx:1259 also needs that check. A small row-logic test with the set populated would guard against this coming back.
There was a problem hiding this comment.
Fixed in 23c6861. ThreadTimeline gets the real pendingApprovals again, so the waiting and liveness rules hold. It no longer gets onRespondToApproval, so TimelinePendingApprovalRow falls back to its existing read-only "Waiting for approval" state. The answer lives only in the composer notice. I checked in the dev server with a seeded command approval: the read-only row renders with no shimmer. The guard pins the real list and the missing responder.
| ...shelfItems, | ||
| forkApprovalNoticeItem({ | ||
| approval: activePendingApproval, | ||
| pendingCount: pendingApprovals.length, |
There was a problem hiding this comment.
Sending a message declines approvals the user can't see.
The notice shows only activePendingApproval and a 1/N count. But getSendContext().shownPendingApprovalIds (line ~3116) still lists every unheld approval, and useComposerSend declines all of them when a message is sent (useComposerSend.ts:985-992). Before this PR, upstream rendered every pending approval in the timeline, so "shown" was accurate. With the timeline rows removed, typing a follow-up while 3 approvals are pending declines approvals 2 and 3, whose commands the user never saw.
Suggested fix: limit shownPendingApprovalIds to activePendingApproval?.requestId inside this fence. Alternatively, keep this behavior on purpose and say so in the notice ("Sending declines all N").
There was a problem hiding this comment.
Fixed in 23c6861. shownPendingApprovalIds is now just activePendingApproval?.requestId (fenced), so a send declines only the approval the notice shows. The guard pins it.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
…n approval The empty approvals list also told the timeline nothing was waiting, so a blocked run showed a live activity row. The timeline gets the real list again but no responder, which leaves a read-only "Waiting for approval" marker. A send now declines only the approval the notice shows, and the not-resumable notice no longer reads as the command. Queued rows name a sender other than the owner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Responses to the code quality review of
Required checks were green on 0350f12. CI is re-running on 23c6861. |
Problem
The 2026-10-28 upstream sync changed two things above the composer. Queued messages became dashed bubbles at the end of the timeline, with "Send now" and no way to reorder them (upstream pingdotgg#25401, pingdotgg#25846). Pending approvals moved onto their timeline row (pingdotgg#26255). We prefer the composer-attached versions.
Change
Queue banner (
fork-queued-runs-banner):custom/ForkQueuedRunsBanner.tsxbrings back the collapsible Queued · N banner in the composer dock. Rows are one line each, with drag-grip and arrow-key reordering, plus Edit, Steer and Remove. It keeps upstream's later behavior:The server still handles
queued-run.reorder, so only the client-runtimereorderQueuedRuncommand comes back, behind fences. ChatView drops the queue from the timeline footer. The user doc describes reordering again.Approval notice (
fork-composer-approval-notice):custom/forkApprovalNotice.tsxbuilds the pending approval as a notice card in the composer banner stack. It uses the fork's notice styling: shield icon, the label over the mono command and the reason, the white Approve primary, then Decline and More. The timeline gets an empty pending-approvals list, so approval rows render as plain work entries. A held approval (while typing) hides as before, and the approval counts as a blocking drawer. A small CSS rule stacks the title over the command in light too.Manifest entries and guard tests for both.
Mobile keeps upstream's queue.
Verification
vp test run src/__fork_guards__/: 87 files, 654 tests pass.vp run check:changed: lint, knip, typecheck and graphql pass, and the fork lint ratchet passes. The first run failed format; I fixed it withvp fmtbefore committing, and the commit hook re-ranvp fmtcleanly. The only test failure is the known Apple Silicon cross-arch case inbuild-desktop-artifact.test.ts, which also fails on pristine upstream.🤖 Generated with Claude Code