Fix batch of bugs: Dockerfile, duplicate HTML attrs, nil guards#68
Fix batch of bugs: Dockerfile, duplicate HTML attrs, nil guards#68
Conversation
There was a problem hiding this comment.
Pull request overview
This PR batches several fixes across the Rails app’s views and containerization to address broken/stale Docker builds, invalid HTML, and analytics rendering edge cases.
Changes:
- Rewrites the Dockerfile to a multi-stage build, removes Node/Yarn, and runs the app as a non-root user.
- Fixes invalid HTML in views (duplicate
classattribute, stray</header>). - Adds nil-guarding/fallback rendering for analytics metadata and corrects error vs warning CSS targeting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Switches to multi-stage build with slimmer runtime image and non-root execution. |
| app/views/mods/_mod.html.erb | Removes duplicate class attribute by merging classes. |
| app/views/layouts/application.html.erb | Fixes malformed header markup by removing an extra closing tag. |
| app/views/mods/_analytics.html.erb | Adds guards/fallback UI for missing analytics data and fixes error styling condition. |
| app/views/mods/show.html.erb | Uses safe navigation when passing analytics metadata to the partial. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/views/mods/show.html.erb
Outdated
| </button> | ||
| <div class="mt-12 min-h-48"> | ||
| <%= render("analytics", metadata: @mod.metadata[:status]) if params[:analytics] %> | ||
| <%= render("analytics", metadata: @mod.metadata&.dig(:status)) if params[:analytics] && @mod.metadata.present? %> |
There was a problem hiding this comment.
The extra && @mod.metadata.present? guard prevents rendering the analytics partial when @mod.metadata is nil, so the partial can’t show its new "No analytics data available" fallback. Consider rendering whenever params[:analytics] is set and pass @mod.metadata&.dig(:status) (letting the partial handle nil).
| <%= render("analytics", metadata: @mod.metadata&.dig(:status)) if params[:analytics] && @mod.metadata.present? %> | |
| <%= render("analytics", metadata: @mod.metadata&.dig(:status)) if params[:analytics] %> |
app/views/mods/show.html.erb
Outdated
| </button> | ||
| <div class="mt-12 min-h-48"> | ||
| <%= render("analytics", metadata: @mod.metadata[:status]) if params[:analytics] %> | ||
| <%= render("analytics", metadata: @mod.metadata&.dig(:status)) if params[:analytics] && @mod.metadata.present? %> |
There was a problem hiding this comment.
This change is intended to make analytics rendering resilient to missing metadata, but the existing request specs only cover the case where metadata[:status] is present. Please add a request spec for analytics=true when @mod.metadata (or metadata[:status]) is nil/blank to assert the fallback behavior (e.g., "No analytics data available").
| # Run as non-root user | ||
| RUN groupadd --system rails && \ | ||
| useradd rails --system --gid rails --home /rails && \ | ||
| chown -R rails:rails /rails | ||
| USER rails:rails |
There was a problem hiding this comment.
The image now runs as the non-root rails user, but the deployment config mounts a named volume at /rails/tmp (config/deploy.yml). Docker volumes are typically owned by root on first create, which can cause runtime failures when Puma/Rails tries to write pid/cache files under tmp/. Consider adding an entrypoint that chowns /rails/tmp at container start (then drops privileges), or adjust the volume/user strategy so /rails/tmp is writable by the rails user.
- Rewrite Dockerfile with multi-stage build, remove unused Node/Yarn (#58) - Fix duplicate class attribute on tr in _mod.html.erb (#59) - Fix duplicate </header> closing tag in layout (#62) - Add nil guards to analytics partial + fix CSS class bug (#64) Closes #58, closes #59, closes #62, closes #64
- New spec file for _analytics.html.erb partial (nil guards, error/warning rendering) - Add nil metadata context to show.html.erb spec - All 230 specs passing
- Remove redundant metadata.present? guard in show view; let the partial handle nil metadata with its fallback message - Add docker-entrypoint.sh to ensure tmp dirs are writable (Docker volumes mount as root, but app runs as non-root user) - Update spec to verify nil metadata shows fallback message
1977677 to
e3b92e9
Compare
Changes
Fixes all 4 identified bugs in one batch:
#58 — Dockerfile rewrite
#59 — Duplicate
classattribute in_mod.html.erbclassattributes into onecursor-pointerand lighter hover styles#62 — Duplicate
</header>in layout</header>that caused a stray closing tag#64 — Analytics partial nil guards + CSS bug
metadata,metadata[:errors],metadata[:warnings]type == :error→type == :errorsso red CSS actually applies to errors&.dig) in show viewCloses #58, closes #59, closes #62, closes #64