This repository was archived by the owner on Sep 17, 2026. It is now read-only.
Repository navigation
Conversation
… LLM Complete the attachment upload so files reach the (vision-capable) LLM via attachment_* turn-context keys (storageRef) instead of inert "[ref:...]" chat text the backend never parsed. - Full attachment API client: upload (rich result incl. forwardableInline), list, download, delete, delete-all; client-side caps; typed AttachmentError; buildAttachmentContext helper. - Thread attachment context through streaming, non-streaming, AND managed-agent sends (the managed path previously dropped context on empty-text turns). - Pending chips (thumbnails, remove, status, "too large to forward" warning), multi-file, attachment-only turns; attachments render on the user bubble and are preserved across undo/redo. - StrictMode-safe upload/remove; object-URL lifecycle (revoke on remove / send / clear / unmount / conversation change); BEM CSS. - Public-widget auth model preserved (same-origin credentials, no token). - ~45 tests incl. streaming context forwarding, error codes, cap enforcement, and store preview cleanup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Member
Author
|
Superseded by Rebasing was not realistic: this branch targets On the crux, this PR was right and is unchanged: Taken from here:
Not taken, with reasons:
One correction to the reasoning in my own port, for the record: the backend does not leave the model unaware of a skipped attachment — Two things this PR identified that remain genuinely open are written up in |
ginccc
added a commit
that referenced
this pull request
Jul 22, 2026
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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ginccc
added a commit
that referenced
this pull request
Jul 22, 2026
…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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ginccc
added a commit
that referenced
this pull request
Jul 22, 2026
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.
ginccc
added a commit
that referenced
this pull request
Jul 22, 2026
…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 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.
1 participant
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.
Summary
Completes the chat widget's attachment upload so files reach the (vision-capable) LLM via
attachment_*turn-context keys (using thestorageReffield the backend prefers) instead of inert📎 file [ref:…]chat text the backend never parsed.What changed
api/chat-api.ts): full lifecycle — upload (rich result incl.forwardableInline), list, download, delete, delete-all; client-side caps; typedAttachmentError;buildAttachmentContext(); broadened context type to carry structuredattachment_*values.chat-api.ts,ChatWidget.tsx): thread attachment context through streaming, non-streaming, and managed-agent sends (the managed path previously dropped context on empty-text turns). Attachments carried on the user message and preserved across undo/redo (revoking only orphaned previews).ChatInput.tsx,MessageBubble.tsx,chat.css): pending chips (thumbnails, remove, status, "too large to forward" warning), multi-file, attachment-only turns, attachments on the user bubble. StrictMode-safe upload/remove; object-URL lifecycle (revoke on remove / send / clear / unmount / conversation change); BEM CSS.Testing
tsc -bclean, full production build passes, full vitest suite green (89 tests).Review
Passed a multi-agent adversarial review across correctness-vs-backend-contract, React/StrictMode state, auth, security, CSS, and test quality; every confirmed finding was fixed.
🤖 Generated with Claude Code