Review · grok-ce-code-review-adversarial-02 · Image attachments in comments and replies

High

[x] AUTO-FIX: Overlapping retry still un-readies a successful upload: reserve_slot! only skips the uploading reset when the row is already ready (ingest.rb:93), and record_failure then update_columns to failed for any draft (ingest.rb:257-262) with no ready guard, so aborting the first XHR (image_attachment_composer_controller.js:201) and retrying the same client_key lets the disconnected attempt mark the completed row failed and block ClaimBatch.
[x] AUTO-FIX: Remove/abort does not stop in-flight ingest: RemoveAttachment destroys the row (remove_attachment.rb:26-29) while commit! only batch.with_locks and then image.attach + update! on the stale object (ingest.rb:174-188), so a concurrent commit can attach a blob to a missing record (polymorphic Active Storage has no FK, orphan reconciliation never sees it) or, if resolveServerId ran before reserve_slot!, leave a ghost ready row that ClaimBatch binds after the composer looks empty.
[x] AUTO-FIX: Typed alt text is dropped on Save: alt is PATCHed only from a fire-and-forget change listener (image_attachment_composer_controller.js:182-184) and onSubmit never flushes or awaits it (:370-394); clicking Save blurs the field and races WriteAltText against claim, and once claim has moved the row off the batch the PATCH is a 404 (write_alt_text.rb:27-28) so the posted message keeps a blank description.
[x] AUTO-FIX: Annotorious cancel still has no discard path after the pass-01 decision: BatchesController is still create/show only (batches_controller.rb:4-14, routes.rb:13-15) and disconnect leaves the batch for 24h (image_attachment_composer_controller.js:67-69), so six attach-then-cancel cycles still raise too_many_open_batches / "Finish or discard one first" (error.rb:37) with nothing to discard, against escalation A1 (idempotent unclaimed-batch DELETE, cancel-only, claim-safe).

Medium

[x] AUTO-FIX: Submit and batch/alt fetch still have no timeout after the upload XHR cap: onSubmit awaits fetch with submitting true (image_attachment_composer_controller.js:378-409) and request() is used for batch create/alt/remove (:447-457) with no AbortController, so a hung claim or ensureBatch POST leaves the composer frozen (refreshSubmitState keeps submit disabled) even though UPLOAD_TIMEOUT exists for the file XHR.
[x] AUTO-FIX: The six-open-batch cap is check-then-create without a user/account lock (image_attachment_batch.rb:38-41,55-58), so two composers posting ensureBatch together can both observe count < 6 and insert extra open batches; the byte ceiling is rechecked at ingest commit, but the documented per-account batch ceiling is not.
[x] AUTO-FIX: Non-JSON posting failures are silent: onSubmit does response.json().catch(() => ({})) and on !ok calls restoreFrom (image_attachment_composer_controller.js:395-404), and showError no-ops on an empty message (:528-529), so a 500/HTML/CSRF body leaves the composer enabled with no error while the catch path for a thrown fetch does tell the user to retry.

Nit

[x] AUTO-FIX: DraftPresenter.failure hard-codes retryable: true for every stored failure (draft_presenter.rb:54), including invalid_image, so the Retry control stays the recovery path for bytes that the server will never accept.