This repository was archived by the owner on Sep 17, 2026. It is now read-only.
Repository navigation
feat(chat-ui): EDDI feature parity since the HITL framework - #29
Merged
Merged
Conversation
Audit of EDDI features merged since and including PR #585 (HITL framework) found 133 verified gaps. Records the verified backend contract read from EDDI source — 6 conversation states, 8 SSE events, the attachment context shape — and breaks the work into P0..P4. Two findings dominate and neither is HITL: attachments never reach the model (storageRef stitched into message text instead of an attachment_* context key), and several always-on stream defects corrupt output for every agent.
Self-review found item 7 too vague to implement from. Pins the detection signal (zero token events + conversationState in the three states ConversationService skips on) and the required behaviour.
…vocabulary
Always-on defects affecting every agent, independent of any HITL config.
SSE parsing (chat-api.ts):
- Stop calling .trim() on each `data:` line. The SSE spec strips exactly one
optional leading space; .trim() then ate the token's own indentation and
trailing spaces, corrupting code blocks and nested markdown.
- Stop defaulting an unnamed frame to type "token". Every unrecognised event
had its payload appended to the agent's message as visible text.
- Ignore `:` comment / keep-alive frames instead of emitting a bogus event.
Event vocabulary (types.ts):
- ConversationState now models all six backend values; AWAITING_HUMAN and
EXECUTION_INTERRUPTED were missing.
- SSEEventType now models all eight emitted events. Removed "thinking", which
the backend never emits — the indicator was only ever cleared by the first
token, so it span forever on a turn that failed or paused before producing
one. It is now driven by task_start/task_complete/task_failed.
HTTP (new http.ts):
- Status-aware core. Non-2xx now throws ApiError carrying status and the
server's body; all 10 call sites previously collapsed to statusText and
discarded the body, which is why 409/403/408 were indistinguishable.
- requestJson tolerates an empty 200 — undo/redo return no body, and
res.json() on that threw, surfacing as a failed undo.
Stream semantics (new sse-events.ts, ChatWidget):
- `error` payload is {"message":"…"}; render the message, not the raw JSON.
- Detect a server-skipped turn (zero tokens + a state ConversationService
skips on). Previously it arrived as an ordinary `done` carrying the PREVIOUS
step's outputs, so stale quick replies were re-offered as new and the empty
bubble rendered "No response".
- Refresh undo/redo availability after a streamed turn; the trimmed `done`
payload omits both flags, leaving the buttons permanently greyed.
75 tests pass (was 46); tsc clean.
The upload half was correct; the reference half was not. ChatInput stitched
the returned storageRef into the message TEXT as "📎 name [ref:…]".
AttachmentContextExtractor only scans context keys prefixed "attachment_" and
requires Context.value to be a Map carrying storageRef — so the file never
reached the model. Upload returned 201, a bubble appeared, and the agent
answered as if nothing had been attached. Silent at every layer.
- New attachments-api.ts owns upload, delete, and context construction.
buildAttachmentContext emits attachment_N: {type:"object", value:{storageRef,
fileName}} and caps at 5 (DEFAULT_MAX_ATTACHMENTS_PER_TURN).
- mimeType/sizeBytes are deliberately NOT sent: for stored blobs the backend
resolves both from validated store metadata and does not trust the client.
- Uploads now stage in store state (pendingAttachments) and travel with the
next message, instead of being sent immediately as text. Staged files render
as removable chips; an attachment alone is a valid turn.
- Upload rejections name the actual reason (ATTACHMENT_TOO_LARGE,
ATTACHMENT_REJECTED, 401/403) instead of a generic failure.
- Widened the context type to value: unknown — the previous `value: string`
could not express the required payload at all.
Note: docs/attachments-guide.md "Path C" tells clients to put the ref in `url`.
That is wrong for uploaded blobs — `url` routes into the URL/SSRF branch which
only accepts http/https. The code is truth.
91 tests pass (was 75); tsc clean; production build succeeds.
The widget had zero HITL vocabulary: a repo-wide grep for hitl|awaiting|approv|pause|resume returned one unrelated doc comment. A paused conversation rendered as a working chat with a live composer, and every subsequent message was refused by the backend with nothing shown to the user. Read-only by design. The widget shows that a reviewer must act and offers a way out, but never renders Approve/Reject — /resume would accept the call (eddi-user is in its @RolesAllowed), but a user approving their own gate defeats the oversight the pause exists to provide. Deciding stays in Manager UI via /agents/pending-approvals. - hitl-api.ts: approval-status, cancel, deadline maths, poll cadence. pausedAt is Instant.toString() (ISO-8601) on the summary endpoint but epoch millis under detail=full snapshot serialization, so both forms are accepted. - useHitlPolling: the 1:1 REST/SSE surface has NO push channel for HITL, so polling is the only way to learn a pause resolved — including when the timeout policy auto-decides. Backs off 3s→30s; survives transient failures. - PausedCard: why the turn stalled, which tools are gated, and when it will auto-decide (naming the policy's actual outcome). - Composer and quick replies are locked while paused, so a message cannot be typed into a conversation that will refuse it. - 409 on send now withdraws the optimistic bubble and hands the text back to the composer — a 409 means the input was never consumed, so silently keeping it in the transcript implied it had been sent. 401/403 get their own message. - Bare-string entries in conversationOutputs[].output[] now render. HITL writes its pending-approval placeholder and reviewer-rejection message as raw Strings; code reading only `.text` dropped them, so a paused or rejected turn showed nothing at all. Also fixes a bug the new ChatWidget tests caught: the paused card rendered only in the non-empty-messages branch, so a conversation that starts ALREADY paused (the CONVERSATION_START gate, commit dc3cd1c81) showed "Starting conversation…" indefinitely. Adds ChatWidget integration tests — nothing imported ChatWidget before, so a syntax error in the largest file could survive a fully green suite. 139 tests pass (was 91); tsc clean; production build succeeds.
P3 — cancel/interruption: - Stop control while a turn is in flight. The SSE reader was unbounded with no way out, so a runaway turn could only be escaped by reloading the page. Aborts the local read first (tokens stop immediately), then POSTs /cancel so the server stops producing them. A 409 (nothing to cancel) is treated as success — the turn finished as the button was clicked. - The paused card's cancel is the same primitive, used as the escape hatch out of AWAITING_HUMAN. P4 — ownership: - setAuthToken attaches a Bearer token to every request, from ChatConfig or a ?token= query param. Conversation ownership is enforced server-side, so an anonymous client can be locked out of the conversation it just started. - Caller-supplied headers are preserved when the token is attached. Also updates AGENTS.md with the current file layout and a "backend contract" section recording the facts verified against EDDI source — six conversation states, eight SSE events, the JSON error payload, the trimmed done snapshot, bare-string output entries, and the attachment context shape. These are the details that were guessed wrong before. 146 tests pass (was 139); tsc clean; production build succeeds.
The test consumes ~5s of real delays but relied on the 5s default timeout, while its sibling already carried an explicit 15000. Under parallel load it timed out intermittently. Gives it the same explicit timeout — the behaviour was never wrong, the budget was.
Two bugs in the P0/P2 work, found re-reading the done handler. 1. isSkippedTurn conflated "the server dropped this turn" with "this turn paused". Both end with zero tokens and conversationState AWAITING_HUMAN, so a turn that was accepted and then gated for approval was reported to the user as "your message was not sent" — false, and it discarded the turn's own output. ConversationService skips based on the state it finds ON ARRIVAL, so the distinguishing signal is the state BEFORE the send. TurnContext now carries it, and isTurnPaused/isSkippedTurn split the two cases apart. 2. The done handler never rendered conversationOutputs text at all. On a streaming turn tokens carry the answer, but a turn with NO tokens has its text only in the done payload — including HITL's pending-approval placeholder, which arrives as a bare string. A paused turn therefore rendered as "No response". Also adds reconciliation of streamed text against the final snapshot: responseValidation `fallback` substitutes its own text AFTER tokens have been streamed, so the user was left reading a response the backend had decided not to give. The snapshot is authoritative and now wins. Drops the dead TurnContext.skipped field. 161 tests pass (was 146); tsc clean.
…undo/redo
ERROR and EXECUTION_INTERRUPTED were dead ends. The widget stored the state but
branched on neither, so a failed or cancelled turn left a live composer whose
every message the backend refused, with no explanation and no recovery. The
only escape was reloading the page.
- Adds rerunLastStep (POST /agents/{id}/rerun), the backend's retry primitive,
which the widget never called.
- Recovery banner for both states offering "Try again" (rerun) and "Start
over" (new conversation), with distinct copy — interrupted is a user-initiated
cancel, ERROR is a failure.
- undo/redo failures are now shown to the user instead of going only to
console.error. This matters because undoAvailable/redoAvailable are
deliberately NOT pause-aware server-side: the buttons can be enabled while
the operation is refused with 409, so the user clicked and nothing happened.
168 tests pass (was 161); tsc clean.
A six-lens review with per-finding adversarial verification confirmed 12 real defects (and correctly refuted 2 findings already fixed earlier on the branch). They reduce to four root causes. HITL polling treated "not AWAITING_HUMAN" as resolution - resumeConversation CASes AWAITING_HUMAN -> IN_PROGRESS BEFORE running the approved turn (ConversationService.java:1326-1335), and approval-status echoes that persisted field. So the widget declared the pause over while the answer was still being generated, tore down the watch, and never fetched the result — the user waited for an approval and then got nothing. Resolution now requires a SETTLED state (READY/ENDED/ERROR/ EXECUTION_INTERRUPTED); IN_PROGRESS keeps watching. - A failed post-resolution refresh permanently stopped the loop, wedging the widget in a paused state with a locked composer. onResolved now reports success and the loop keeps polling until it confirms. The optimistic bubble was treated as the source of truth for retrying - WITHDRAW_LAST_USER_MESSAGE restored the bubble's DISPLAY content, so a 409 handed back the secret mask "●●●●●●●●" (destroying the real secret) or text prefixed with "📎 filename" lines. - Staged attachments were cleared before the request, so a refused send lost them with no retry path. handleSend now records the raw turn and withdraw restores both text and attachments. Position was used to find the streaming bubble - A local notice (upload failure, cap warning) dispatched mid-turn became "the last message", so every later token, the finish, and the reconcile landed in the ERROR bubble. Targeting is now by identity (isStreaming), and the attach button is disabled while a turn is in flight. Managed-agent mode never had a conversationId - It never calls startConversation, so HITL polling, cancel, retry and attachments were all inert on that route, and a paused managed conversation locked the widget with no controls at all. processSnapshot now adopts the conversationId every snapshot carries. Also: rerunLastStep omitted the `language` query param. The backend declares it with NO @DefaultValue and calls checkNotEmpty before any other work, so the retry button added in the previous commit was an unconditional 400 — it could never have worked once. The two new useHitlPolling regression tests were confirmed to fail against the pre-fix hook and pass after. 188 tests pass (was 168); tsc clean; production build succeeds.
A second adversarial review, scoped to the code written since the first one
(which was itself unreviewed), confirmed 12 defects and refuted 2. Three of the
four HIGH findings were introduced BY the previous round of fixes.
HIGH — the recovery banner never fired for the case it exists for
A failed streaming turn sends `error` and closes with NO `done`, so nothing set
conversationState; the post-stream refresh had `conversationState: "ERROR"` in
hand and discarded it. On the default transport the widget still believed it was
READY, so Try again / Start over were unreachable. The refresh now adopts the
authoritative state.
HIGH — the managed-agent route silently dropped every attachment
sendManagedAgentMessage had no `context` parameter at all, so the ONLY ingestion
path the backend reads was discarded while the transcript showed "📎 file". The
previous commit made this reachable by enabling the attach button there. It also
chose its verb from the truthiness of `message`, so an attachment-only turn
issued a GET — sending nothing and re-reading the conversation. Split into
loadManagedConversation (GET) and sendManagedAgentMessage (always POST, with
context).
HIGH — a 409 put a plaintext secret into the unmasked composer
The previous fix ("restore the raw text, not the mask") overshot: it handed the
real secret back as visible text with its secret marking lost. A withdrawn
secret turn now restores nothing.
MEDIUM
- The prior-state guard on isSkippedTurn was too broad. The skip is a race
against the PERSISTED state, which the client cannot observe, so requiring a
locally-observed skip state made detection unreachable. IN_PROGRESS in a
turn's own `done` can only mean a drop; only AWAITING_HUMAN is ambiguous.
- Mid-resume IN_PROGRESS blanked the paused card and its Cancel button for the
whole resumed turn. The status is no longer cleared while the turn runs.
- handleRetry and the post-approval refresh both re-read the SAME step and
appended it again, duplicating the transcript. Snapshot-derived messages now
carry a stable sourceKey and are idempotent.
- A non-409 send failure left an empty bubble stuck in isStreaming forever.
- onResolved applied its snapshot even after the watch was torn down, so a
restart adopted the old conversation's transcript. It now receives a
staleness check.
- pendingTurnRef was a single slot and WITHDRAW targeted lastIndexOf("user"), so
a late-failing turn withdrew a newer, unrelated message. Per-turn state is now
keyed by message id and the withdraw targets that exact message.
Refuted and left alone: the claimed skipped-turn regression (the trigger does not
exist — same-conversation turns are serialized by conversationCoordinator), and
rerun's hardcoded language=en (the transition is null -> "en", provably inert in
both consumers).
Every fix was checked by reverting it and confirming the new test fails. One
integration assertion was found to pass vacuously (it raced the restore effect);
it was corrected and backed with a deterministic reducer test.
205 tests pass (was 188); tsc clean; production build succeeds.
Round 3 reviewed the previous fix commit and confirmed 5 defects (refuting 4). Two HIGH findings were the same regression, introduced BY the previous round's fix — the third consecutive round in which a fix created a fresh defect. HIGH — the duplicate-transcript fix swallowed genuinely new replies Every non-streaming response is read with returnCurrentStepOnly=true, which reduces conversationOutputs to a single element (ConversationMemoryUtilities :168). outputIndex was therefore always 0 and the key degenerated to the reply text truncated at 64 chars. Any repeated agent utterance — a fallback, a re-prompt, a confirmation, or two answers sharing a long preamble — collided, and the reducer dropped the second one. The user sent a message, their bubble appeared, and no answer ever arrived. This hit the whole managed-agent route (always non-streaming) and any enableStreaming:false deployment, and was a regression: before the previous commit both replies rendered. Dedupe is now opt-in and applied ONLY by the two callers that deliberately re-read an already-rendered step (handleRetry, handlePauseResolved), which is all it was ever meant to cover. Ordinary sends append as before. The key also no longer truncates the text. MEDIUM - The post-stream refresh raced Stop: handleStop cancels then sets EXECUTION_INTERRUPTED, and a refresh issued earlier landed afterwards with a stale READY, wiping it — and the recovery banner with it. The refresh no longer speaks for the state when its AbortController is no longer current. - A dropped turn left the user's own bubble in the transcript, asserting the message had been sent when it never reached the agent. It is now withdrawn and the text handed back. - The managed route's post-409 re-read was gated on environment && agentId, which that route never has, so it never learned it had been refused. Also fixes a leak found before the review: pendingTurnsRef entries were released only on failure paths, so a long session accumulated one record (text plus attachments) per successful turn. 207 tests pass (was 205); tsc clean; build succeeds. Both new tests confirmed to fail with the regression reinstated and pass after.
… placeholder Found by self-review while subagents were rate-limited. The previous commit's duplicate-suppression could not work for the one case it was written for. The pending-approval placeholder reaches the transcript through the streaming `done` handler, which dispatches plain ADD_MESSAGE and stamps no sourceKey. The post-approval refresh then re-read the same step and compared by sourceKey — `some(m => m.sourceKey === key)` against undefined is always false — so the placeholder was appended a second time. The round-2 finding it was meant to fix was therefore still live. ADD_SNAPSHOT_MESSAGE now suppresses on agent-message CONTENT, which works regardless of which path first rendered the text. This is safe precisely because dedupe became opt-in in the previous commit: only handleRetry and handlePauseResolved use it, and both deliberately revisit a step already shown. Ordinary sends still append unconditionally, so a repeated fallback is not swallowed. sourceKey is removed entirely — it no longer carries meaning, and leaving it would invite a future reader to trust it. 212 tests pass (was 207); tsc clean; build succeeds. The new integration test was confirmed to fail against the key-based comparison and pass after.
…ript wipe
Round 4 reviewed the entire branch and confirmed 13 defects, 9 high/critical.
The root cause behind several of them — and behind four rounds of green suites
hiding real bugs — was that this client's types and test fixtures described a
backend wire shape that does not exist.
CRITICAL — undo/redo erased the whole conversation
`ConversationStep` declared `{input, output}`. The real shape is
`{conversationStep: [{key, value}], timestamp}` (SimpleConversationMemorySnapshot
:46-65). Both fields were therefore always undefined, the rebuild produced an
empty list, and REPLACE_MESSAGES blanked the transcript. On master this was
unreachable — res.json() threw on undo's empty 200 body first — so the earlier
requestJson fix is what made it destructive.
- types.ts now models the real shape.
- New snapshot.ts maps conversationStep entries by key (input:initial -> user,
output* -> agent) with its own tests.
- The rebuild is guarded: an empty result means "could not rebuild", never
"the conversation is empty".
- processSnapshot read the same fictional fields; also fixed.
- Test fixtures rewritten to the real wire shape. Doing so immediately failed
two previously-green tests, which is the point.
HIGH — the SSE fix was still wrong, in the other direction
resteasy-reactive's SseUtil.serialiseField writes `field + ":" + value` with NO
delimiter space (SseUtil.java:84). Stripping one leading space per the SSE spec
deleted a space that belongs to the token — and LLM streams emit " word"
constantly, so words ran together. Now taken verbatim; fixtures use real framing.
HIGH — an abandoned stream wrote into the conversation that replaced it
New Conversation never aborted the in-flight turn. Added a generation counter:
handleRestart aborts, bumps it, and every async continuation bails when it moves.
HIGH — QuickReplies bypassed the busy guard
The composer disables itself, but QuickReplies and SecretInput call handleSend
directly, so a click mid-turn started a second one. Guard moved into handleSend.
HIGH — recovery/undo/redo were inert on the managed route
They required environment+agentId, which that route does not have and the v6 API
does not use. conversationId is the real precondition.
HIGH/MEDIUM — snapshot dedupe
Scoped to the trailing run of agent messages (a repeat from an earlier turn is
always separated by a user bubble), and matches a blank-line-delimited segment
so the `done` handler's joined bubble and the refresh's per-entry messages can
compare at all.
LOW — Stop before the first token left an empty bubble rendering "No response".
225 tests pass (was 212); tsc clean; build succeeds. The undo, SSE and dedupe
fixes were each confirmed by reverting them and watching the new tests fail.
…rebuild
Both defects were introduced by the previous commit and confirmed by the final
whole-branch review.
CRITICAL — every snapshot rendered twice
processSnapshot treated conversationOutputs and conversationSteps as
alternative response formats. They are not: the backend fills BOTH from the same
memory for every response (ConversationMemoryUtilities:172-209), and only nulls
one out when the caller passes returningFields, which this client never sends.
The steps block was inert while it read the fictional {input, output} shape;
correcting that shape made it live, so every reply rendered twice and the user's
own input was echoed back beneath it. conversationSteps is now a FALLBACK
(`else if`) for snapshots carrying no outputs.
HIGH — the rebuild printed secret input in clear
`input:initial` holds the RAW message even for a secret turn: Conversation.java
:337 stores it unmasked and masks only conversationOutput["input"], which the
wire filter then drops. So rendering a step's input re-displayed the password
the composer had shown as ●●●●●●●●. The widget now records the texts it sent
with secret mode on and masks them on any rebuild.
Residual limitation, stated rather than hidden: that record is session-scoped.
After a page reload the widget cannot know which past turns were secret, so a
rebuild of an older conversation can still surface them. Masking those properly
requires the backend to stop returning input:initial in clear, or to flag the
turn — this client cannot fix it alone.
Test fixtures now include a realistic snapshot with BOTH lists populated. Every
previous fixture set one or the other, which is exactly why a double-render
could not be caught. Both new tests were confirmed to fail against the previous
code and pass after.
229 tests pass (was 225); tsc clean; build succeeds.
The final review's remaining findings reduce to two mechanical gaps once the previous commit's double-render and secret-masking fixes are accounted for. The generation counter guarded only the streaming loop. New Conversation during an in-flight managed send, non-streaming send, undo, redo or retry let the abandoned turn's continuation write its snapshot into the conversation that replaced it. Every continuation now captures the generation and re-checks it before dispatching. ADD_SNAPSHOT_MESSAGE compared content without regard to role, and its window covered only the trailing agent run. A snapshot re-read carries the user's input:initial as well as the outputs, so the user echo could never be matched and was appended again. The window now includes the trailing user bubble and comparison is role-matched, so a user echo can only suppress a user bubble and an agent reply only an agent one. 232 tests pass (was 229); tsc clean; build succeeds.
Self-contained instructions for (1) the EDDI-side fix for input:initial returning secret input in clear — which the widget can only paper over session-scoped — and (2) the optional undo/redo-off-by-default rollout. Written so someone without this branch's history can act on either.
|
Warning Review limit reached
Next review available in: 35 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 Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis change aligns the chat client with the EDDI contract by adding shared HTTP and SSE handling, snapshot reconstruction, attachment uploads, HITL polling and pause UI, richer reducer state, cancellation and recovery flows, managed-agent updates, accessibility changes, and comprehensive API, reducer, component, and integration tests. ChangesEDDI feature parity
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatWidget
participant ChatInput
participant AttachmentsAPI
participant ChatAPI
participant EDDI
User->>ChatInput: select files and enter message
ChatInput->>AttachmentsAPI: upload attachments
AttachmentsAPI->>EDDI: POST conversation attachment
EDDI-->>AttachmentsAPI: attachment metadata
ChatInput-->>ChatWidget: staged attachments
ChatWidget->>ChatAPI: send message with attachment context
ChatAPI->>EDDI: POST streaming request
EDDI-->>ChatAPI: SSE task, token, cascade, done, or error events
ChatAPI-->>ChatWidget: parsed stream events
ChatWidget-->>User: transcript, pause, recovery, or completion state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
ginccc
force-pushed
the
feat/chat-ui-eddi-feature-parity
branch
from
July 22, 2026 10:53
561fb30 to
eec9b19
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the React chat widget’s client-side contract and UI flows to match EDDI backend behavior introduced since the HITL framework, focusing on wire-fidelity (SSE + HTTP status), attachments via attachment_N context entries, and paused/error recovery handling.
Changes:
- Introduces a status-aware HTTP core (
ApiError, bearer token support) and updates API calls + SSE parsing/interpretation accordingly. - Implements attachments as staged chips that are sent via object-valued
attachment_Ncontext entries (with a 5/turn cap) instead of embedding refs in message text. - Adds HITL read-only UX: paused card + approval-status polling, cancel/stop support, and recovery UX for
ERROR/EXECUTION_INTERRUPTED.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Expands shared types for context maps, full conversation states, and SSE event types. |
| src/test-utils/sse.ts | Adds SSE stream and fetch mocking utilities for tests. |
| src/test-setup.ts | Adds jsdom polyfill for scrollIntoView. |
| src/styles/chat.css | Adds styles for attachment chips, paused card, and recovery banner. |
| src/store/chat-store.tsx | Extends store with pending attachments, HITL status, draft restore, and streaming-safe reducer actions. |
| src/store/chat-store.test.tsx | Adds reducer coverage for new actions (attachments, skipped turns, draft restore, dedupe, reconcile). |
| src/hooks/useHitlPolling.ts | Adds polling hook for approval-status with backoff and stale-guarding. |
| src/hooks/useHitlPolling.test.ts | Tests polling behavior and resolution semantics. |
| src/components/PausedCard.tsx | Adds read-only paused UI for awaiting approval, including deadline text and cancel. |
| src/components/PausedCard.test.tsx | Tests paused card rendering, accessibility, and “no approve/reject” policy. |
| src/components/ChatWidget.tsx | Major integration: attachment context wiring, HITL UX, stop/cancel, snapshot processing, SSE semantics, recovery banner. |
| src/components/ChatWidget.test.tsx | Adds integration smoke/regression tests for new behaviors (paused, undo safety, stop, managed route, dedupe). |
| src/components/ChatInput.tsx | Implements staged attachment uploads, cap enforcement, and 409 draft restore behavior. |
| src/components/ChatInput.test.tsx | Tests attachment staging/chips and attachment-only sends. |
| src/api/sse-events.ts | Adds pure helpers for interpreting SSE done/error, skipped-turn detection, and output extraction. |
| src/api/sse-events.test.ts | Unit tests for SSE interpretation helpers. |
| src/api/snapshot.ts | Adds snapshot→transcript mapping for the backend’s real step shape, including secret masking. |
| src/api/snapshot.test.ts | Tests transcript rebuild against the real wire shape and secret masking behavior. |
| src/api/http.ts | New status-aware request helpers + bearer token support. |
| src/api/http.test.ts | Tests auth header behavior and ApiError body/status surfacing. |
| src/api/hitl-api.ts | New HITL API functions + deadline/tool gating helpers + polling cadence helpers. |
| src/api/hitl-api.test.ts | Tests duration parsing, deadline computation, polling delay, and cancel/status calls. |
| src/api/demo-api.ts | Updates demo SSE simulation to use real backend event vocabulary. |
| src/api/demo-api.test.ts | Adjusts timing to reduce flakiness under test load. |
| src/api/chat-api.ts | Refactors API layer onto http.ts, fixes SSE parsing, managed conversation loading/sending, retry, undo/redo empty-body handling. |
| src/api/chat-api.test.ts | Adds tests for SSE whitespace fidelity, empty-body handling, and managed attachment context forwarding. |
| src/api/attachments-api.ts | New attachment upload/delete API + attachment context construction. |
| src/api/attachments-api.test.ts | Tests attachment context contract and upload error surfacing. |
| grep.exe.stackdump | Adds a crash dump file (should not be in the repo). |
| docs/superpowers/specs/2026-07-21-followups-handoff.md | Documents follow-ups, including backend secret-input exposure caveat. |
| docs/superpowers/specs/2026-07-21-chat-ui-eddi-feature-parity-design.md | Design/audit doc for parity work and backend contract. |
| AGENTS.md | Updates assistant guidelines to reflect new modules and backend contract notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Compared the two other open PRs against this branch. #27 (model-cascade hint) and #28 (multimodal attachments) are now superseded and can be closed; what was genuinely missing is here, and what they got wrong is deliberately not. Attachments — three real gaps, all verified against the EDDI Java: - forwardableInline. The upload cap and the forward cap are different limits (20 MiB vs 10 MiB). A file in between is stored, returns 201, and is then dropped at forward time with a ForwardSkipException. That skip lands in attachments:errors, which every writer marks setPublic(false) — so the turn can never reveal it and the agent simply answers as if no file existed. The upload response is the only signal we get; the chip and the sent bubble now say so. - deleteAttachment had zero callers. Removing a chip dropped it client-side only, leaving the blob in the store against a 50-file / 100 MB per-conversation quota until nothing further could be attached. - Error copy named the wrong cause. ATTACHMENT_REJECTED is a catch-all over MIME rejection, both quotas and empty files; we printed "that file type is not accepted" for all of them. errorPayload() now parses the {error, code} envelope and prefers the server's own text, which is the actionable half. Also: multi-file select with a batch-aware cap (the picker was single-file while the cap is 5), placeholder chips while bytes are in flight, an empty fileName is omitted so the extractor's own backfill can fire, and the chip strip is a live region — an upload failure posts into the transcript, which is not announced. Cascade — ported the "Thinking harder…" hint, not the commit. A buffered cascade emits its whole answer as one token, so the wait after an escalation is silent. Two things in #27 stay out: its unguarded SET_THINKING on cascade_step_start defeats this branch's tokenCount guard (a guaranteed-accept step can stream live, time out, and be followed by the next step's start), and it is built on master's phantom `thinking` event, which this branch removed as non-existent. The reset lives in the FINISH_STREAMING reducer so every exit path is covered at once. Also fixes an unguarded continuation: a stream abandoned by New Conversation dispatched FINISH_STREAMING into the conversation that replaced it — the guard sat one statement too low. Currently benign, so it has no dedicated test. Untracks grep.exe.stackdump and git-log-output.txt, committed by accident. Deliberately not taken, with reasons in the handoff doc: #28's object-URL lifecycle (this branch creates none), its positional undo/redo preservation (silently fails on redo), its reducer-side revocation, its hardcoded 20 MiB client pre-check (the limit is deployment config), and its list/download/ delete-all endpoints (dead exports in #28 too, and the download URL cannot carry this branch's bearer token). 254 tests (was 232), tsc --noEmit clean, production build succeeds. Each new fix was checked by reverting it and confirming the matching test fails.
…t was wrong Three independent reviews of 497a2b7 (correctness, UX/a11y, tests+contract). Most of that commit holds; these are the parts that did not. ## A regression, not a hardening Moving the generation guard above the abandoned stream's FINISH_STREAMING was NOT benign, contrary to what the last commit claimed. CLEAR_MESSAGES never cleared isProcessing, and that safety net was the only thing that did — so New Conversation mid-stream left the FRESH conversation with a disabled composer, a dead Enter key and a Stop button for a turn that was not running. Fixed where it belonged all along: CLEAR_MESSAGES now clears isProcessing. Two more from the same review: - The composer could send while an upload was in flight. The turn went out with no attachment context, CLEAR_ATTACHMENTS fired, and the upload landed as a chip for the NEXT turn — a file the user believed they had just sent. Made worse by this branch's concurrent batch upload, which widened the window from one file to five. - The fire-and-forget DELETE targeted the CURRENT conversation. An upload started before New Conversation lands afterwards and stages into the fresh composer; deleting it was refused by the store's owner check and the blob leaked against the quota this was meant to protect. AttachmentResult now keeps the conversationId the upload reported. ## Prose that was wrong The last commit justified forwardableInline with a claim that does not survive reading the Java, and put it in AGENTS.md under "do not guess these": - `setPublic(false)` is NOT a wire filter. `isPublic()` is never read as a predicate anywhere in EDDI — it is only copied into the snapshot. The real gate is a key-prefix allowlist that `returnDetailed=true` opens wholesale. - The model is NOT left unaware. AttachmentForwarder substitutes `TextContent.from(skipMessage)`, telling it the file was not sent and that readAttachment can fetch it. "The agent answers as if no file existed" was the opposite of what the code does. - forwardableInline is an incomplete signal: a separate aggregate forward cap can skip a file that reported true. - cascade_escalation carries no model name (only cascade_step_start does) and no cost at all — the Indicators comment and a test name both said otherwise. - ATTACHMENT_REJECTED is a declared-vs-detected MIME *mismatch*, not an unaccepted type: there is no allowlist. On Postgres it also wraps any SQLException, so a database outage reaches the user as a "rejection". - The per-turn cap of 5 is a @ConfigProperty default. The last commit rejected PR #28's hardcoded size pre-check on exactly that ground while hardcoding this. Kept, but the asymmetry is now argued rather than ignored. Copy follows the corrected facts: "too large to send directly", not a claim the assistant cannot read it. ## UX and accessibility - The live region was useless: it mounted together with its content (never announced) and an upload failure only ever REMOVES a chip, which the default aria-relevant does not cover. Now always mounted and explicitly written to. - The warn chip hid what it was warning about. A nowrap flex item cannot shrink, so the warning beat the file name — 44 characters ellipsised to ~14. Measured in a browser at 320px: now ~40, on its own row. - Five chips could squeeze the transcript to nothing (307px of composer). Capped and scrolling at 88px. - Remove button 18px → 24px (WCAG 2.2 AA), 36px on coarse pointers; it now fires an irreversible delete, so a mis-tap is no longer client-only. - Focus went to <body> on chip removal; now moves to the next chip or the attach button. Warning text associated via aria-describedby, in-flight chip gets aria-busy and a text label, emoji are aria-hidden, duplicate file names disambiguate. - ⚡ read as *fast* for a state that means *slower*; "harder" implied the first attempt was half-hearted. Now 🧠 "Taking a closer look…" — and folded into ThinkingIndicator as a prop so React stops remounting and replaying the entrance animation mid-wait. - Attach button no longer disabled by any in-flight upload, only by the cap. - prefers-reduced-motion honoured. ## Tests Mutation testing found five of six mutations survived the previous suite, including deleting the `multiple` attribute that IS the multi-file feature and repointing deleteAttachment at the upload endpoint with the wrong verb. So the last commit's "verified by reverting each fix" was true only of the reverts actually performed. All five are now caught, re-verified by re-applying each. Note jsdom does not enforce `multiple` — fireEvent.change hands over several files regardless — so that attribute is pinned by direct assertion and cannot be covered behaviourally here. 274 tests (was 254), tsc --noEmit clean, production build succeeds.
This was referenced Jul 22, 2026
Member
Author
Update: folded in #27 and #28, which are now closedTwo commits pushed. The other two open chat-UI PRs covered overlapping ground, so they were compared hunk by hunk against this branch and against the EDDI Java rather than merged (#28 targets #27's branch, not 2ad34f0 — the port. Three real attachment gaps this branch had:
Plus multi-file select with a batch-aware cap, in-flight chips, and omitting an empty c322ead — review round. Three independent reviews. Worth flagging honestly:
Tests: 232 → 274. Mutation testing found five mutations surviving the 2ad34f0 suite — including deleting the Also removes Two items remain genuinely open and are written up in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/components/ChatWidget.tsx (1)
181-189: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSync these refs in an effect rather than during render.
conversationStateRef.current = …(Line 182) andisProcessingRef.current = …(Line 189) write to refs during render. React 19 render must stay pure — a replayed or discarded render can leak these writes. Since both refs are read only at click time (event handlers), an effect update is timing-equivalent and keeps render pure.Proposed change
- const conversationStateRef = useRef(state.conversationState); - conversationStateRef.current = state.conversationState; + const conversationStateRef = useRef(state.conversationState); ... - const isProcessingRef = useRef(state.isProcessing); - isProcessingRef.current = state.isProcessing; + const isProcessingRef = useRef(state.isProcessing); + + useEffect(() => { + conversationStateRef.current = state.conversationState; + isProcessingRef.current = state.isProcessing; + }, [state.conversationState, state.isProcessing]);Please verify against the target React version that no click-time read depends on the pre-commit (render-phase) value; effect-synced refs update after commit.
🤖 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 `@src/components/ChatWidget.tsx` around lines 181 - 189, Move the assignments updating conversationStateRef.current and isProcessingRef.current out of render and into an effect in the ChatWidget component, using the committed state values as dependencies. Keep the existing event-handler reads and guard behavior unchanged, since both refs are only needed at click time after effects have run.Source: Linters/SAST tools
src/styles/chat.css (1)
1360-1360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a CSS custom property instead of the hardcoded
#d9822b.Every other color in this stylesheet comes from a
--chat-*custom property; this literal orange bypasses theming and can't be overridden. Consider introducing a token (e.g.--chat-warning) with a fallback.As per coding guidelines: "Use CSS custom properties for styling and keep component styles in the shared CSS".♻️ Proposed fix
- border-left: 3px solid `#d9822b`; + border-left: 3px solid var(--chat-warning, `#d9822b`);🤖 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 `@src/styles/chat.css` at line 1360, Replace the hardcoded `#d9822b` in the border-left declaration with a --chat-* CSS custom property, adding a --chat-warning token with an appropriate fallback if no existing warning token is available, so the color remains theme-overridable.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 `@docs/superpowers/specs/2026-07-21-followups-handoff.md`:
- Around line 81-86: Narrow the documentation claim in “Persist the secret flag
per step and honour it in the projection” to state that only non-detailed
projections mask input:initial, while detailed responses may retain plaintext.
Define the intended trust boundary and add an acceptance check for each detailed
endpoint that is required to scrub secret input.
- Around line 208-217: Update the attachment URL revocation effect to diff the
previous and current URL sets, revoking only URLs absent from the new set when
attachments change. Preserve URLs retained by the current render, and revoke the
complete remaining set only during unmount cleanup; keep reducer logic pure.
In `@src/api/sse-events.test.ts`:
- Around line 188-190: Rename the test case around isSkippedTurn to describe
that transitioning from READY to ENDED is not treated as a skipped turn, while
preserving its existing false assertion and behavior.
In `@src/components/PausedCard.test.tsx`:
- Around line 27-103: Wrap every PausedCard render in the test suite with
ChatProvider, importing it from the chat store module as needed. Update all
render calls in the PausedCard describe block, including nested multiline
renders, while preserving the existing props and assertions.
In `@src/styles/chat.css`:
- Around line 1247-1258: Update the .chat-sr-only accessibility utility to
replace the deprecated clip declaration with the equivalent clip-path
declaration, preserving its visually hidden and screen-reader-announced behavior
while satisfying stylelint.
---
Nitpick comments:
In `@src/components/ChatWidget.tsx`:
- Around line 181-189: Move the assignments updating
conversationStateRef.current and isProcessingRef.current out of render and into
an effect in the ChatWidget component, using the committed state values as
dependencies. Keep the existing event-handler reads and guard behavior
unchanged, since both refs are only needed at click time after effects have run.
In `@src/styles/chat.css`:
- Line 1360: Replace the hardcoded `#d9822b` in the border-left declaration with a
--chat-* CSS custom property, adding a --chat-warning token with an appropriate
fallback if no existing warning token is available, so the color remains
theme-overridable.
🪄 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
Run ID: 528cca7f-6795-404f-998b-db9dcb53f0d3
📒 Files selected for processing (34)
.gitignoreAGENTS.mddocs/superpowers/specs/2026-07-21-chat-ui-eddi-feature-parity-design.mddocs/superpowers/specs/2026-07-21-followups-handoff.mdgit-log-output.txtsrc/api/attachments-api.test.tssrc/api/attachments-api.tssrc/api/chat-api.test.tssrc/api/chat-api.tssrc/api/demo-api.test.tssrc/api/demo-api.tssrc/api/hitl-api.test.tssrc/api/hitl-api.tssrc/api/http.test.tssrc/api/http.tssrc/api/snapshot.test.tssrc/api/snapshot.tssrc/api/sse-events.test.tssrc/api/sse-events.tssrc/components/ChatInput.test.tsxsrc/components/ChatInput.tsxsrc/components/ChatWidget.test.tsxsrc/components/ChatWidget.tsxsrc/components/Indicators.tsxsrc/components/PausedCard.test.tsxsrc/components/PausedCard.tsxsrc/hooks/useHitlPolling.test.tssrc/hooks/useHitlPolling.tssrc/store/chat-store.test.tsxsrc/store/chat-store.tsxsrc/styles/chat.csssrc/test-setup.tssrc/test-utils/sse.tssrc/types.ts
💤 Files with no reviewable changes (1)
- git-log-output.txt
…tion Verified the escalation indicator renders in a browser rather than only in the DOM: both states distinct, label contrast 5.85:1 dark / 5.74:1 light, 🧠 painted, and pulse correctly suppressed under prefers-reduced-motion so the brain rests at opacity 1 instead of its 0.4 trough. That check made the media query look incomplete — it covers .indicator__brain and .indicator__dot but not the msgIn entrance. That is deliberate: the infinite animations are the ones that keep moving in the reader's periphery for a whole turn, a one-shot 250ms entrance is not. Written down so it does not read as an oversight. Comment only, no rule changes.
…buttals ## Real - **parseIsoDuration silently dropped the day component.** The regex matched `(?:\d+D)?` and never captured it, so an approval window of `P1DT1H` was reported as one hour instead of 25 — the deadline shown to the user was a day early. It also required the `T` section, so a bare `P2D` (which the backend's Duration.parse accepts) matched nothing and showed no deadline at all. Days are now captured and the time section is optional. Four new cases; verified by reverting the fix and watching them fail. - **The recovery banner's warning colour was hardcoded `#d9822b`,** which is 2.93:1 on the light surface — under the 3:1 minimum for a non-text indicator. Now a themed `--chat-warning`, darkened to `#a85a12` for light (5.08:1) and unchanged at 6.30:1 for dark. - **Refs were assigned during render.** conversationStateRef and isProcessingRef were written in the render body, so a render React discards would leave them holding state the user never saw, and handleSend guards on exactly those. Moved to an effect — every reader is a click handler, which cannot run before commit, so it is timing-equivalent and pure. ## Nits - `.chat-sr-only` gains `clip-path` alongside the deprecated `clip`. - An `isSkippedTurn` test was named "detects a drop into an ended conversation" while asserting the opposite; the behaviour was right, the name was copied from the IN_PROGRESS case above it. Renamed and explained. - PausedCard tests now mount inside ChatProvider, per the repo convention. It renders purely from props today; this keeps them valid if that changes. - Handoff doc: the object-URL advice said to revoke from a cleanup "keyed on the URL set", which would revoke still-visible previews on every change — removing one of five chips would blank the other four. Now says to diff the sets and revoke only what left. Also narrowed the secret-input claim: option 1 masks only the non-detailed projection, and returnDetailed=true bypasses the allowlist, so the acceptance check has to cover both. ## Not taken - **undoAvailable/redoAvailable defaults are correct as they stand.** They are deliberately asymmetric and per-handler: after a successful undo, redo IS available (`redoAvailable ?? true` inside handleUndo), and after a redo, undo IS available. Defaulting both to false would grey out the one button the user just earned. - **No stylelint is configured** in this repo — the deprecated-`clip` rule cannot fail a lint that does not run. Fixed on merit, not for the stated reason. - **grep.exe.stackdump was already removed** in the first commit of this series; that comment predates the push. 278 tests (was 274), tsc --noEmit clean, production build succeeds.
ginccc
force-pushed
the
feat/chat-ui-eddi-feature-parity
branch
from
July 22, 2026 15:47
bca329d to
68950f8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the chat widget up to the EDDI backend features merged since and including the HITL framework (PR #585, 2026-07-13): HITL, multimodal attachments, model cascade, MCP ownership, group follow-ups, and error handling/recovery.
Why
An audit against the backend found the widget silently wrong on several always-on paths, not just missing HITL. Highlights:
📎bubble appeared, but thestorageRefwas stitched into the message text;AttachmentContextExtractoronly readsattachment_*context keys. Silent at every layer.data:line. RESTEasy'sSseUtil.serialiseFieldwritesfield + ":" + valuewith no delimiter space, so leading spaces belong to the token — and LLM streams emit" word"constantly.AWAITING_HUMANwas unmodelled, so the widget showed a typing indicator indefinitely while the backend refused every subsequent message.statusTextand discarded the body, making 409/403/408 indistinguishable.What's in it
errorpayloads, status-awarehttp.tswithApiErrorattachment_Nobject-valued context, staged chips, rejection codes, 5/turn capapproval-statuspolling with backoff, paused card, 409 preserves inputPOST /cancel,EXECUTION_INTERRUPTED/rerunretry + restart banner forERROR/EXECUTION_INTERRUPTEDHITL is deliberately read-only here — the widget surfaces a paused turn and offers cancel, but never Approve/Reject.
/resumewould accept the call, but a user approving their own gate defeats the oversight the pause exists to provide. Deciding belongs in Manager UI via/agents/pending-approvals.Correctness notes
The client previously described a backend wire shape that does not exist:
ConversationStepdeclared{input, output}, whereasSimpleConversationStepis{conversationStep: [{key, value}], timestamp}. Undo/redo rebuilt the transcript from those fields, always got[], and erased the conversation. Types and test fixtures now model the real shape — doing so immediately failed two previously-green tests.The branch went through five adversarial review rounds. Each finding was verified against the EDDI Java rather than this client's types, and each fix was checked by reverting it and confirming the corresponding test fails.
Verification
tsc --noEmitclean, production build succeedsFollow-ups
See
docs/superpowers/specs/2026-07-21-followups-handoff.md:input:initialreturns secret input in clear.Conversation.java:330-343stores the raw message and masks onlyconversationOutput["input"], which the wire projection then drops. The widget masks session-scoped only — after a reload it cannot know which past turns were secret. This needs a backend change.stepsToMessagesrebuilds frominput:initial, so one undo strips it from every prior turn. feat(chat-ui): multimodal attachments — forward uploaded files to the LLM #28 fixed this by list position, which silently fails on redo — not taken.Filein component state, this branch's store holds onlyAttachmentResult).Summary by CodeRabbit
New Features
Documentation
Style