Add photo upload tab with ActiveStorage + MinIO (#33) #36
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "33-upload-tab-minio"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
Upload feature
app/models/upload.rb: Upload model withhas_one_attached :photoapp/controllers/uploads_controller.rb: index, show, create, edit, update, destroy with:see_otherredirectsapp/views/uploads/index.html.erb: single Upload Photo button, auto-submit on file select, compact thumbnail listapp/views/uploads/show.html.erb: full-size view with upload date, Edit Caption + Deleteapp/views/uploads/edit.html.erb: edit caption formapp/javascript/controllers/upload_controller.js: Stimulus controller for auto-submit on file selectionStorage
Gemfile: addedaws-sdk-s3for MinIOGemfile.lock: updatedconfig/storage.yml: MinIO S3-compatible service config via env varsconfig/environments/production.rb: auto-selects MinIO whenMINIO_ENDPOINTis setdb/migrate/: ActiveStorage tables + uploads tabledb/schema.rb: updatedProperties improvements
app/views/properties/manage.html.erb: Show Inactive filter chip (hidden by default)app/views/properties/edit.html.erb: Active checkbox on edit formapp/controllers/properties_controller.rb: permit:active, add:see_otherredirectapp/javascript/controllers/filter_controller.js: inactive toggle filter, run filter on connectNavigation + styling
app/views/layouts/application.html.erb: Photos tab (5th tab) in bottom navapp/assets/stylesheets/application.css: upload list, buttons, show page, and edit caption stylesconfig/routes.rb:resources :uploadswith full CRUDDocs
docs/app-architecture.md: Upload model, MinIO in diagrams, design decisiondocs/ROADMAP.md: Ticket 4 sectionInfra (pal-e-deployments, committed separately)
X-Forwarded-Proto: httpsfor Tailscale funnelproxy_redirectto rewrite upstream Location headerslandscaping-uploadscreated on clusterlandscaping-assistant-secretspatched with MinIO credentialsTest Plan
Review Checklist
Related Notes
ldraney/landscaping-assistant #33-- Upload tab: snap and store schedule paper photo in MinIOlandscaping-assistant-- project this work belongs toCloses #33
PR #36 Review
DOMAIN REVIEW
Tech stack: Ruby on Rails 8.1 / ActiveStorage / Hotwire (Stimulus + Turbo) / PostgreSQL / MinIO (S3-compatible) / CSS (vanilla, no framework)
Rails / ActiveStorage
config/storage.yml) correctly uses env vars for all credentials, withforce_path_style: truefor MinIO. Good.config/environments/production.rb) conditionally selects:miniovs:localbased onMINIO_ENDPOINTpresence. Clean approach.Uploadmodel validates:photopresence. ActiveStorage migration is the standard Rails-generated one. Schema is correct.UploadsControlleruses strong params with two separate param methods (upload_paramsfor create,caption_paramsfor update). Good separation -- prevents photo replacement on caption edits.status: :see_other(303), which is correct for Turbo Drive compatibility. This also fixes the existingPropertiesController#updatewhich was missing it.newaction, which makes sense since the form is inline onindex).Content type validation gap: The
Uploadmodel validates presence of:photobut does NOT validate content type or file size. Theaccept: "image/*"attribute on the file input is client-side only -- it can be bypassed. A user (or attacker) could upload arbitrary files (executables, multi-GB files) through the endpoint. Rails 7+ ActiveStorage supportsvalidates :photo, content_type: [...]andvalidates :photo, size: { less_than: N.megabytes }natively. This is not a security blocker in a single-user app with no auth, but it is a correctness and resource-safety issue.N+1 query risk:
UploadsController#indexloads all uploads and then iterates in the view callingupload.photo.attached?andupload.photo.variant(...). Without eager loading (Upload.with_attached_photo), this generates N+1 queries. For a small photo collection this is fine, but it should useUpload.with_attached_photo.order(created_at: :desc)as a best practice.Stimulus / JavaScript
upload_controller.jsis minimal and correct.requestSubmit()properly fires form validation (vssubmit()which would skip it).filter_controller.jscleanly extends the existing controller with a new target and action. Theconnect()hook ensures inactive properties are hidden on page load, which is correct behavior.CSS
var(--spacing-*),var(--color-*),var(--radius)) consistently with the existing design system. No magic numbers for spacing/colors..btn-delete:hoverand.btn-danger-outline:hoverboth usebackground: #fef2f2instead of a CSS variable. Not a blocker, but should use a theme token for consistency.Views / UX
turbo_confirm. Good.show.html.erbline 9:image_tag @upload.photorenders the full original image. For a phone camera photo this could be several MB. Consider using a variant for display (e.g.,resize_to_limit: [1200, 1200]) to reduce bandwidth on mobile.image_tagcalls. Screen readers will not have context for these images. The thumbnail call on index line 21 and the full image on show line 9 should includealt:attributes (e.g.,alt: upload.caption.presence || "Schedule photo").Documentation
docs/app-architecture.md,docs/ROADMAP.md,docs/hotwire.md,docs/infrastructure-and-pipeline.md,docs/networking.mdall updated to reflect the new feature. Thorough.BLOCKERS
1. No test coverage for new Upload functionality
The project has an established RSpec test suite (CI runs
bundle exec rspec). There are existing model specs (spec/models/property_spec.rb) and request specs (spec/requests/properties_spec.rb). This PR adds a new model (Upload), a new controller (UploadsController) with 6 actions, and modifiesfilter_controller.js-- none of which have tests.Required at minimum:
spec/models/upload_spec.rb: validates photo presence, valid with photo attached, optional captionspec/requests/uploads_spec.rb: create with photo, create without photo (failure), show, edit, update caption, destroyThis is a BLOCKER per review criteria: "New functionality with zero test coverage."
NITS
N+1 query on uploads index:
UploadsController#indexshould useUpload.with_attached_photo.order(created_at: :desc)to eager-load attachments.Hardcoded hover color:
.btn-delete:hoverand.btn-danger-outline:hoveruse#fef2f2-- consider adding a--color-danger-lighttoken.Missing alt text on images: Both the thumbnail in
uploads/index.html.erb(line 21) and the full image inuploads/show.html.erb(line 9) should includealt:attributes for accessibility.No content type or size validation on Upload model: Consider adding
validates :photo, content_type: ['image/png', 'image/jpeg', 'image/webp', 'image/heic'], size: { less_than: 20.megabytes }to prevent non-image uploads and oversized files server-side.Full-size image on show page:
image_tag @upload.photoserves the raw upload. On mobile, camera photos are typically 3-8 MB. A display variant (resize_to_limit: [1200, 1200]) would improve load times.PR checklist incomplete: "Passed automated review-fix loop" is unchecked, and "CI pipeline green" is unchecked. These should be resolved before merge.
SOP COMPLIANCE
33-upload-tab-miniofollows{issue-number}-{kebab-case-purpose}conventionPROCESS OBSERVATIONS
ENV["MINIO_ENDPOINT"].present?fallback to:localis a safe degradation path.bundle exec rspec, so adding specs will catch regressions automatically. Without them, the upload CRUD has zero automated safety net.VERDICT: NOT APPROVED
One blocker: no test coverage for the new Upload model and controller. The project has an established RSpec test suite and CI pipeline that runs tests. New CRUD functionality (6 controller actions, 1 model) must have specs before merge. Fix the blocker and address the nits (especially N+1 eager loading and alt text), then re-request review.
PR #36 Review (Re-review)
Previous review: NOT APPROVED (blocker: no test coverage; nits: N+1, missing alt tags, no content type/size validation, hardcoded CSS color, raw full-size image on show). This re-review confirms whether all items were addressed.
DOMAIN REVIEW
Stack: Ruby on Rails 8.1 / ActiveStorage / ActiveRecord / Stimulus JS / Vanilla CSS
Model layer (
app/models/upload.rb):ALLOWED_CONTENT_TYPESconstant -- covers PNG, JPEG, WebP, HEIC/HEIF. Previous nit resolved.MAX_FILE_SIZE = 20.megabytes. Previous nit resolved.if: -> { photo.attached? }) correctly avoids callingcontent_type/byte_sizeon nil. Good.has_one_attached :photois appropriate for single-file uploads.Controller layer (
app/controllers/uploads_controller.rb):with_attached_photoeager loading on index -- eliminates the N+1 query. Previous nit resolved.status: :see_otherfor Turbo Drive compatibility. Correct.upload_params(photo + caption) andcaption_params(caption only). The update action correctly usescaption_params, preventing photo replacement via PATCH. Good separation.:unprocessable_entity. Correct.Views:
index.html.erb: Thumbnails usevariant(resize_to_fill: [80, 80])-- no raw full-size images in list. Previous nit resolved.show.html.erb: Full-size view usesvariant(resize_to_limit: [1200, 1200])-- caps resolution rather than serving raw originals. Previous nit resolved.altattributes with meaningful fallbacks (upload.caption.presence || "Schedule photo"). Previous nit resolved.turbo_confirmfor destructive actions. Good.CSS (
application.css):--color-danger-light: #fef2f2token added. Both.btn-remove:hoverand.flash-alertnow reference the token instead of the hardcoded hex value. Previous nit resolved.--spacing-*,--radius,--color-*). No magic numbers introduced.Storage (
config/storage.yml):force_path_style: trueneeded for MinIO S3-compatible endpoints. Correct.ENV["MINIO_ENDPOINT"]. Reasonable fallback.Tests:
spec/models/upload_spec.rb: 5 examples covering valid photo, missing photo, rejected content type, file size limit, and optional caption. Covers happy path + edge cases + error handling. Previous blocker resolved.spec/requests/uploads_spec.rb: 7 examples covering all CRUD routes (GET index, POST create success/failure, GET show, GET edit, PATCH update, DELETE destroy). Verifies status codes, redirects, and database state. Previous blocker resolved.config/environments/test.rb:config.hosts.clearadded to allow test requests. Necessary for RSpec request specs.Properties improvements:
:activeadded to strong params inPropertiesController. Appropriate.filter_controller.js:connect()callsfilter()so inactive properties are hidden on page load.toggleInactiveaction wired correctly.is-inactiveclass conditionally and the Stimulus controller reads it. Clean integration.BLOCKERS
None. All previous blockers have been addressed:
NITS
All previous nits have been addressed. New observations (non-blocking):
Caption XSS surface:
upload.captionis rendered via ERB<%= %>which auto-escapes in Rails. No issue, just noting the review was done.No
newroute: Routes areonly: [:index, :show, :create, :edit, :update, :destroy]-- no standalonenewaction. The create form is embedded inindex.html.erb. This is intentional for the single-page upload flow but worth noting for future developers.Stimulus upload controller is minimal: The entire controller is
this.element.requestSubmit(). If this pattern is needed elsewhere, it could become a genericauto-submitcontroller. Not worth changing for a single use.SOP COMPLIANCE
33-upload-tab-miniomatches issue #33PROCESS OBSERVATIONS
see_otherredirect pattern is applied consistently.VERDICT: APPROVED