Skip to content
Hive

screenote · Image attachments in comments and replies

Review

reviews/codex-ce-code-review-04.md

High

  • [x] RESOLVED/NO-FIX: Preserve drafts when a composer is cancelled (app/javascript/controllers/image_attachment_composer_controller.js:633): cancel() sends DELETE requests for committed draft rows, contradicting the required 24-hour recovery behavior for Annotorious cancellation and disconnects. <!-- triage: prior escalation A1 requires purge on explicit Annotorious cancel and preservation only on disconnect/navigation -->
  • [x] AUTO-FIX: Bind claimed-batch replay to the intended composer target (app/services/image_attachments/claim_batch.rb:63): replay validates only the parent class, so a reply batch claimed on one annotation can be reused from another reply or reopen composer and return the wrong existing comment. <!-- triage: plan requires one isolated batch per composer -->
  • [x] AUTO-FIX: Replay a successful reopen before checking current annotation state (app/controllers/annotation_comments_controller.rb:9): the resolved-state guard now returns 422 before the claimed batch can return its existing comment, breaking response-loss idempotency after the first reopen succeeds. <!-- triage: plan explicitly requires response-loss replay -->
  • [x] AUTO-FIX: Return the created or replayed comment ID on comment success (app/controllers/annotation_comments_controller.rb:12): the controller discards ClaimBatch's parent and returns only annotation_id, so reply and reopen responses do not identify the parent required by the submission contract. <!-- triage: plan defines the success parent identity -->
  • [x] AUTO-FIX: Include editable form state in 422 rehydration payloads (app/controllers/concerns/image_attachment_submission.rb:66): invalid root, reply, and reopen submissions return only batch metadata and ready IDs, omitting the required body and root coordinates/viewport needed to restore user input. <!-- triage: plan defines the complete 422 payload -->
  • [x] AUTO-FIX: Renew batch activity atomically with the ready transition (app/services/image_attachments/ingest.rb:185): the batch lock is released before touch_activity!, allowing cleanup to destroy a newly ready upload in the intervening window. <!-- triage: lifecycle invariant requires one locked transition -->
  • [x] AUTO-FIX: Prevent late ingest failures from overwriting a successful retry (app/services/image_attachments/ingest.rb:285): mark_failed! checks state and then performs an unconditional update, so a concurrent retry can commit ready between those operations and be reverted to failed. <!-- triage: conditional state update is the clear race fix -->
  • [x] AUTO-FIX: Reject trailing-payload image polyglots (app/services/image_attachments/ingest.rb:146): the current libvips open-and-average path accepts a valid PNG followed by arbitrary payload bytes, so it does not meet the required full-decode and polyglot rejection contract. <!-- triage: R4 explicitly requires polyglot rejection -->
  • [x] AUTO-FIX: Verify recurring cleanup in the live supervisor (app/services/screenote/recurring_tasks.rb:30): readiness only parses and memoizes config/recurring.yml, so it stays healthy when Solid Queue is not actually running or has not registered the cleanup task. <!-- triage: deployment health contract requires live supervisor proof -->
  • [x] AUTO-FIX: Honor explicit dark contexts in the gallery viewer (app/assets/stylesheets/image_attachments.css:6): the viewer redeclares its own light variables and shadows inherited component-context values, leaving explicit dark galleries/viewers light unless the OS preference is dark. <!-- triage: explicit component dark styling is in scope -->
  • [x] AUTO-FIX: Keep submission idempotency active until navigation completes (app/javascript/controllers/image_attachment_composer_controller.js:615): success clears the batch and re-enables the still-mounted form before asynchronous Turbo navigation finishes, allowing a second click to create an attachment-free duplicate message. <!-- triage: straightforward duplicate-submission race -->
  • [x] AUTO-FIX: Ignore stale batch-create responses after reconnect (app/javascript/controllers/image_attachment_composer_controller.js:80): disconnect neither aborts nor generation-gates requestBatch(), so an older response can overwrite the reconnected controller's batch and upload invisible stale items. <!-- triage: reconnect contract forbids stale requests -->
  • [x] AUTO-FIX: Snapshot form fields only after pending alt-text writes finish (app/javascript/controllers/image_attachment_composer_controller.js:589): FormData is captured before awaited PATCH requests, so body or coordinate edits made during that wait are visibly retained but an older value is submitted. <!-- triage: move the snapshot after the awaited writes -->
  • [x] AUTO-FIX: Allow byte-valid extensionless images through browser ingest (app/javascript/controllers/image_attachment_composer_controller.js:167): client validation rejects an empty File.type before server sniffing, despite the explicit requirement that extensionless PNG, JPEG, and WebP files pass. <!-- triage: Unit 1 explicitly requires extensionless images -->
  • [x] AUTO-FIX: Make batch creation recoverable after response loss (app/controllers/image_attachment_drafts/batches_controller.rb:5): every retry creates a new batch because the request carries no idempotency key or resume identity, consuming the six-batch allowance when a committed response is lost. <!-- triage: response-loss recovery is a defined draft contract -->
  • [x] AUTO-FIX: Reconcile blobs when purge_later cannot enqueue (app/models/image_attachment.rb:39): attachment destruction can commit before the purge job enqueue fails, leaving an unattached blob that the row-based orphan reconciliation job cannot discover. <!-- triage: lifecycle contract forbids primary or derivative leaks -->
  • [x] AUTO-FIX: Add the required root/reply/reopen multi-image authoring matrix (test/system/image_attachments_test.rb:56): the suite submits only root picker and single-image reply/reopen cases while drop/paste merely attach, leaving the mandated picker/drop/paste posting, retry/blocking, and complete viewer behavior unproved. <!-- triage: Verification Contract mandates this matrix -->
  • [x] AUTO-FIX: Fail browser evidence collection on trace errors and stale output (test/system/application_system_test_case.rb:94): trace start/save failures are rescued, while script/attachment_browser_evidence reuses its output directory and accepts any one trace, so a stale or partial run can satisfy the gate. <!-- triage: evidence gates must fail closed -->
  • [x] AUTO-FIX: Require the PostgreSQL lifecycle workflow in release gates (.github/rulesets/main.json:31): the concurrency workflow is absent from both the main required checks and bin/release-validate, despite the plan making PostgreSQL race and constraint evidence mandatory. <!-- triage: Verification Contract makes PostgreSQL mandatory -->
  • [x] AUTO-FIX: Assert real overlap in PostgreSQL concurrency tests (test/integration/image_attachment_concurrency_test.rb:72): the helper releases the first transaction immediately after starting the second without proving it reached a blocked operation, so serial execution can pass and no concurrent 50 MB aggregate race is exercised. <!-- triage: plan requires real aggregate-race evidence -->
  • [x] AUTO-FIX: Add full MCP and CLI golden attachment payloads (test/tools/get_annotation_tool_test.rb:18): MCP records only a key allowlist and CLI tests deserialize {id: 5}, so neither proves preserved pre-change values plus canonical root and per-comment attachment objects as required. <!-- triage: Unit 6 requires full golden payloads -->

Medium

  • [x] AUTO-FIX: Bound retained removal tombstones (app/services/image_attachments/remove_attachment.rb:48): every novel client key creates a permanent removed row excluded from the five-file cap, allowing one open batch to accumulate unbounded rows and make claim/cleanup lock work grow indefinitely. <!-- triage: bounded draft work is an explicit lifecycle requirement -->
  • [x] AUTO-FIX: Use one lock order for user deletion and ingest (app/models/user.rb:24): dependent batch destruction can lock a batch before the user while ingest locks the user before the batch, creating a recoverable but user-visible database deadlock during account deletion. <!-- triage: plan mandates fixed lock ordering and deadlock absence -->
  • [x] AUTO-FIX: Bound orphan-variant candidate work before grouping (app/jobs/image_attachment_orphan_reconciliation_job.rb:79): the SQL LIMIT is applied after joining and grouping all historical submitted attachments, so each hourly bounded job can still scan an ever-growing table. <!-- triage: bounded reconciliation has a clear query fix -->

Nit

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