Skip to content
Hive

screenote · Image attachments in comments and replies

Review

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

High

  • [x] AUTO-FIX: Failed drafts cannot be removed from the composer, so submit stays blocked: ingest creates the row before streaming, but 4xx/5xx JSON never returns attachment.id; remove() only DELETEs when item.serverId is set, so a GIF/truncated/decoder-busy file that the user clicks Remove still occupies a batch slot and ClaimBatch rejects with attachments_not_ready (image_attachment_composer_controller.js:268, ingest.rb:79, attachments_controller.rb:10). <!-- triage: confirmed against claim_batch.rb:86 — reserved row survives client-side removal; return the reserved attachment id on error (or allow removal by client_key) -->
  • [x] AUTO-FIX: Scheduler-independent draft caps do not count expired unreclaimed batches: enforce_outstanding_draft_caps! filters expires_at > Time.current, so when Solid Queue is down a user can create six new batches every 24h while old open rows and blobs remain (batches_controller.rb:23). <!-- triage: defeats the plan's explicitly scheduler-independent backstop; drop the expiry filter -->
  • [x] AUTO-FIX: Retry of an already-ready client_key resets the row to uploading and can replace the blob: reserve_slot! always update!(state: :uploading) for an existing key, so a lost 201 plus Retry (or overlapping POSTs) clobbers a ready draft and can leak the previous blob (ingest.rb:86). <!-- triage: plan requires idempotent retry and response-loss recovery; return an already-ready row unchanged -->
  • [x] RESOLVED/NO-FIX: JSON 422 rehydration cannot show the rows that actually block claim: composer_state returns only ready_attachment_ids, so restoreFrom never resurfaces failed/uploading server rows the client already dropped (image_attachment_submission.rb:69). <!-- triage: plan Unit 3 step 4 specifies exactly "batch public ID, and ready attachment IDs"; the blocked-submit symptom is covered by High #1 -->

Medium

  • [x] AUTO-FIX: Multipart uploads treat request.content_length as the per-file size: a legitimate ~20MB PNG/JPEG/WebP plus FormData wrapping exceeds MAX_FILE_SIZE and 422s file_too_large before stream_to! measures the file (attachments_controller.rb:15). <!-- triage: pass the uploaded file's own size, not the envelope length -->
  • [x] AUTO-FIX: The 250MB outstanding-draft byte cap is only checked at batch create: six empty batches can then be filled to 50MB each (300MB) because Ingest#commit! never rechecks MAX_OUTSTANDING_DRAFT_BYTES (batches_controller.rb:32, ingest.rb:237). <!-- triage: recheck the outstanding-byte cap inside the existing commit! batch lock -->
  • [x] AUTO-FIX: Submit does not freeze picker/drop/paste: enqueue ignores this.submitting, so a file added during the claim POST can leave the batch non-ready and 422 the post (image_attachment_composer_controller.js:113 / :305). <!-- triage: early-return from enqueue while submitting -->
  • [x] AUTO-FIX: Alt-text PATCH does not lock the batch before update, so a concurrent claim can write submitted metadata (or 500 on the immutability validation) instead of 404 (attachments_controller.rb:26). <!-- triage: stale image_attachment_batch_id_in_database skips submitted_metadata_is_immutable; lock and recheck usable? in the fixed lock order -->
  • [x] AUTO-FIX: Viewer initial focus targets a non-focusable <img> (no tabindex), so keyboard/AT focus does not land on the dialog content as R7 requires (image_attachment_viewer_controller.js:31, _gallery.html.erb:41). <!-- triage: R7 requires initial focus; add tabindex="-1" to the viewer image -->
  • [x] AUTO-FIX: Stimulus maxTotalSize is declared and rendered but never used, so the client can upload a 50MB-violating set and only fail on the later server commit (image_attachment_composer_controller.js:15). <!-- triage: wire the declared value into enqueue -->
  • [x] AUTO-FIX: ProjectScope.annotations eager-loads attachment blobs for list reads that never serialize attachments, paying N attachment queries on GET /api/v1/screenshots/:id/annotations (project_scope.rb:35). <!-- triage: plan Unit 6 step 2 keeps lists metadata-light; split a detail scope (cost is constant extra queries, not N) -->
  • [x] AUTO-FIX: System tests named for keyboard/focus, mixed paste, and drop do not assert those behaviors (no Arrow/Escape/focus-restore, no textarea text, no drop onto form vs canvas), so R7/R10/R13 evidence is a silent pass (image_attachments_test.rb:84, :183). <!-- triage: missing assertions for plan-required R7/R10/R13 behavior -->
  • [x] AUTO-FIX: Draft batch POST (and alt PATCH) have no CSRF rejection test even though Unit 2 requires session POST/PATCH/DELETE without the token to fail (batches_controller_test.rb:13). <!-- triage: plan Unit 2 step 1 names POST/PATCH/DELETE; only POST-upload and DELETE are covered -->
  • [x] AUTO-FIX: PostgreSQL qualification never overlaps cleanup with claim/remove despite the plan and wiki claiming that race (image_attachment_concurrency_test.rb:83). <!-- triage: remove-vs-claim is covered at :83, but the cleanup-vs-claim race the verification contract names is missing -->
  • [x] AUTO-FIX: get_annotation tool description still says only "cropped image … base64" and does not mention attachment metadata/URLs, so MCP clients will not discover the new read contract (get_annotation_tool.rb:5). <!-- triage: doc fix; the envelope itself already carries attachments -->

Nit

  • [x] AUTO-FIX: Failed composer items are styled with [data-state="failed"] instead of a BEM modifier (application.css:2756). <!-- triage: CLAUDE.md mandates BEM naming -->
  • [x] RESOLVED/NO-FIX: Viewer zoom sets imageTarget.style.scale inline, against the project no-inline-styles rule (image_attachment_viewer_controller.js:87). <!-- triage: repo precedent — annotorious_controller.js:397,572 sets .style for dynamic geometry; the rule targets static markup styling -->
  • [x] AUTO-FIX: Go Attachment slices use json:",omitempty", which would drop the always-present empty attachments array if a caller remarsals typed structs (CLI annotation get currently prints raw JSON) (types.go:91). <!-- triage: plan Unit 6 step 1 makes attachments always present; keep the field emitting an array, not null -->
  • [x] AUTO-FIX: DraftPresenter failure copy hardcodes "20MB"/"50MB" instead of ImageAttachment::MAX_* (draft_presenter.rb:13). <!-- triage: derive from the constants so copy cannot drift -->
  • [x] AUTO-FIX: Overlay hint omits the 50MB per-message total that the server enforces (_composer.html.erb:21). <!-- triage: surface the limit the server actually enforces -->

← 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.