Skip to content
Hive

screenote · Image attachments in comments and replies

Review

reviews/grok-ce-code-review-02.md

High

  • [x] AUTO-FIX: Composer dragover requires dataTransfer.files.length > 0, so real OS drops never attach: hasImageFiles() ANDs types with imageFilesFrom(files), but browsers leave files empty until drop; without preventDefault() on dragover the browser navigates to the file instead. System drop tests dispatch drop with a prefilled DataTransfer and never exercise dragover, so R3/R10 composer-scoped drop is a silent pass (image_attachment_composer_controller.js:81-86, :111-116; test/system/image_attachments_test.rb:279-291). <!-- triage: confirmed at :111-113 — gate dragover on types only, filter files on drop, and add a dragover-then-drop system assertion -->
  • [x] AUTO-FIX: Remove during an in-flight upload can still bind the image: remove() aborts, then DELETEs only if serverId or a resume GET already sees the row, always drops the item locally, and upload() discards a 2xx when superseded without DELETE; a POST that reserves after that GET (or a DELETE that loses the race with commit!) leaves a ready row that ClaimBatch will attach (image_attachment_composer_controller.js:228, :318-333; ingest.rb:181). <!-- triage: confirmed — commit! never rechecks the row still exists under the batch lock; re-resolve/reload inside the lock and DELETE a superseded 2xx -->
  • [x] AUTO-FIX: Client timeout/retry can mark an already-ready draft failed: overlapping POSTs share client_key; a later attempt may commit! ready while the original request still hits record_failure, which reloads and update_columns to failed whenever the row is still a draft — blocking submit with attachments_not_ready after the composer showed Ready (ingest.rb:39, :253-262). <!-- triage: confirmed — draft? is only "has a batch", so a ready row passes the guard; add a state_ready? guard to record_failure -->
  • [x] AUTO-FIX: Default screenote annotation list (no --screenshot) remarsals metadata-light REST rows through Annotation and emits attachments: [], asserting "no images" for messages whose attachments exist only on detail reads; agents that treat the empty array as authoritative will skip annotation get / media fetches (internal/screenote/types.go:85-91, :107; internal/cli/annotation.go:67-68). <!-- triage: confirmed — contract_serializer.rb:99 omits attachments on list rows while the Go MarshalJSON re-adds []; plan Unit 6 step 2 keeps lists URL-free, so omit the key on list rows (pointer field) and keep it always-present on detail reads -->

Medium

  • [x] AUTO-FIX: Typed alt text is not part of submit: it is PATCHed only on change and saveAltText is not awaited (and fetch does not throw on 4xx), so Save without blur or a PATCH that loses the claim lock posts the message with alt_text still null (image_attachment_composer_controller.js:182-184, :357-367, :370-394). <!-- triage: confirmed — flush and await pending alt-text writes in onSubmit before claiming, and surface non-ok PATCH responses -->
  • [x] AUTO-FIX: Concurrent enqueue() calls recompute room from this.items.size then await upload(), so overlapping paste/drop/picker can create more than five local items; extras fail server-side and leave the composer blocked until the user removes them (image_attachment_composer_controller.js:129-159). <!-- triage: confirmed — reserve item slots synchronously before awaiting any upload -->
  • [x] AUTO-FIX: too_many_open_batches / draft_storage_exhausted tell the user to "Finish or discard one first", but there is no batch-discard endpoint — only posting a message or waiting out 24-hour expiry — so the actionable copy is false (error.rb:36-37; routes.rb:12-16). <!-- triage: pass-01 escalation A1 already ruled an idempotent unclaimed-batch discard in; implementing it makes the copy true -->
  • [x] AUTO-FIX: Decode is not serialized one-file-at-a-time per batch: Ingest#decode! takes only the global two-slot ImageDecoding::Guard, so overlapping uploads on one batch (retry + in-flight, or concurrent enqueue) can occupy both decoder slots and return decoder_busy for screenshot work (ingest.rb:142-156; plan lifecycle invariant). <!-- triage: plan lifecycle invariant states "Serialize decode one file at a time per batch" and the code does not — plan-required, not deferred -->
  • [x] AUTO-FIX: ImageAttachmentBatch.open_for! checks the six-batch / 250MB caps without a per-user lock, so concurrent composer POSTs can exceed those scheduler-independent ceilings by one request each (image_attachment_batch.rb:38-61). <!-- triage: confirmed check-then-create; plan requires the caps "fail closed" — serialize on the user row inside a transaction -->
  • [x] AUTO-FIX: Variant prewarm runs as perform_later after the claim transaction with no later rewarm: a lost enqueue leaves submitted galleries on the stable placeholder until a process restart, and ImageAttachmentThumbnailJob is not in the orphan-reconciliation path (claim_batch.rb:34, :117-121; image_attachment_thumbnail_job.rb:19-36). <!-- triage: plan Unit 5 step 2 requires idempotent generation-aware reconciliation; ReconcileScreenshotProcessingJob is the existing precedent to mirror -->
  • [x] AUTO-FIX: System test "an invalid post keeps the composer, the body, and the ready image" never asserts the textarea still holds the 5001-character body, so R11 overlay rehydration evidence is still a silent pass on the named behavior (test/system/image_attachments_test.rb:145-158). <!-- triage: confirmed — add the missing textarea-value assertion for clearly defined R11 behavior -->

Nit

  • [x] AUTO-FIX: DraftPresenter.failure hardcodes retryable: true for every stored code while Error#retryable? is only decoder_busy / storage_unavailable; resume JSON and live raises disagree (draft_presenter.rb:54; error.rb:12). <!-- triage: confirmed — derive retryable from Error::RETRYABLE_CODES so resume and raise agree -->
  • [x] AUTO-FIX: New attachment CSS (~327 lines) is appended onto application.css instead of a sibling feature stylesheet like annotorious.css (application.css from the /* Image attachments block). <!-- triage: mechanical move; annotorious.css/auth.css/landing.css are the existing sibling-stylesheet precedent -->
  • [x] AUTO-FIX: Session and API attachment media controllers copy-paste the same private, no-store / Range / send_blob_stream body already present in MediaController (image_attachment_media_controller.rb:56-66; api/image_attachment_media_controller.rb:43-54). <!-- triage: confirmed near-identical bodies across all three controllers — extract one streaming concern, no behavior change -->
  • [x] AUTO-FIX: Model test "submitted metadata and alt text are immutable" only assigns alt_text; media_type / dimensions / byte_size / state / parent FKs are unasserted (test/models/image_attachment_test.rb:60-66). <!-- triage: confirmed — extend the test to the metadata the name already claims -->
  • [x] AUTO-FIX: Mixed-paste system test only checks that dispatchEvent was not cancelled; it cannot prove the textarea received "pasted words" (test/system/image_attachments_test.rb:96-119). <!-- triage: confirmed — assert the textarea value after the paste rather than only the cancellation flag -->

← Back to task

HiveDev, when it’s ready.

HiveDev is the hosted product being built around open-source Hive. It isn’t available yet; join the waitlist to hear when it is. We’ll use your email for HiveDev availability updates.

Read our privacy notice.