Install Hotwire, migrate inline code to Stimulus + CSS tokens #4

Merged

ldraney merged 4 commits from hotwire into main 2026-05-24 22:43:17 +00:00

Conversation 1 Commits 4 Files changed 11 +509 -151

ldraney commented 2026-05-24 18:20:33 +00:00

Owner

Copy link

Summary

• Ran importmap, turbo, and stimulus installers to wire up the Hotwire stack
• Migrated all inline styles into application.css with design tokens from ~/ror-css-guide
• Migrated all inline JS into location_controller.js Stimulus controller
• Added name, services, and notes fields to the property form

Changes

• app/assets/stylesheets/application.css -- design tokens, reset, component styles replacing empty manifest
• app/views/properties/index.html.erb -- clean ERB with Stimulus data attributes, no inline code
• app/views/layouts/application.html.erb -- added javascript_importmap_tags
• app/controllers/properties_controller.rb -- resolve accepts client_name, special_notes, service_ids
• app/javascript/controllers/location_controller.js -- GPS detection, address resolve, recent properties
• app/javascript/application.js -- imports turbo and stimulus controllers
• config/importmap.rb -- pins for turbo, stimulus, controllers
• docs/hotwire.md -- documents setup, migration, and CSS conventions

Test Plan

• Load app at 0.0.0.0:9999 — form fields visible
• Click Find My Location — street display appears on GPS success
• Fill house number + name, select services, add notes, Save Property — record created
• Recent list updates after save
• No inline styles or script blocks remain in ERB

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive

Related Notes

• landscaping-assistant -- project
• Closes #3

## Summary - Ran importmap, turbo, and stimulus installers to wire up the Hotwire stack - Migrated all inline styles into `application.css` with design tokens from `~/ror-css-guide` - Migrated all inline JS into `location_controller.js` Stimulus controller - Added name, services, and notes fields to the property form ## Changes - `app/assets/stylesheets/application.css` -- design tokens, reset, component styles replacing empty manifest - `app/views/properties/index.html.erb` -- clean ERB with Stimulus data attributes, no inline code - `app/views/layouts/application.html.erb` -- added `javascript_importmap_tags` - `app/controllers/properties_controller.rb` -- resolve accepts client_name, special_notes, service_ids - `app/javascript/controllers/location_controller.js` -- GPS detection, address resolve, recent properties - `app/javascript/application.js` -- imports turbo and stimulus controllers - `config/importmap.rb` -- pins for turbo, stimulus, controllers - `docs/hotwire.md` -- documents setup, migration, and CSS conventions ## Test Plan - [ ] Load app at 0.0.0.0:9999 — form fields visible - [ ] Click Find My Location — street display appears on GPS success - [ ] Fill house number + name, select services, add notes, Save Property — record created - [ ] Recent list updates after save - [ ] No inline styles or script blocks remain in ERB ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `landscaping-assistant` -- project - Closes #3

ldraney added 1 commit 2026-05-24 18:20:33 +00:00

Install Hotwire and migrate inline code to Stimulus + CSS tokens ... c305e9226d

Ran importmap, turbo, and stimulus installers. Moved all inline styles
from index.html.erb into application.css using design tokens from
~/ror-css-guide. Moved all inline JS into a Stimulus location controller.
Added name, services, and notes fields to the property form.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-05-24 18:22:49 +00:00

Author

Owner

Copy link

PR #4 Review

DOMAIN REVIEW

Tech stack: Rails 8.1.3, Propshaft, importmap-rails, Hotwire (Turbo + Stimulus), PostgreSQL, plain CSS with design tokens.

Hotwire/Stimulus patterns -- well done:

• Clean Stimulus target wiring; all 12 targets declared and used correctly.
• data-action="click->location#detect" and data-action="click->location#save" follow canonical Stimulus action syntax.
• CSS visibility toggled via .is-visible class rather than inline style.display -- correct separation of concerns.
• CSRF token now read from the meta tag and sent with fetch -- an improvement over the prior inline JS which omitted it entirely.
• connect() lifecycle hook used properly for localStorage hydration.
• eagerLoadControllersFrom + pin_all_from is the standard Rails 8 importmap setup. Correct.

Hotwire/Stimulus patterns -- issues:

1. skip_forgery_protection still active on resolve (line 2 of properties_controller.rb).
   The Stimulus controller now correctly sends X-CSRF-Token, which means CSRF protection should work. But skip_forgery_protection only: [:resolve] disables it for that exact action. This was a necessary workaround when the inline JS did not send the token, but now that the Stimulus controller sends it, this line should be removed. Leaving it means CSRF protection is still bypassed on the only mutating endpoint in the app. This is a security concern but not a BLOCKER per the criteria below since there is no new unvalidated user input introduced -- the bypass existed before this PR.

2. renderRecent() uses .innerHTML with unsanitized property data (line 121 of location_controller.js).
   address_line and city come from user input via the Nominatim API response. If a malicious value gets stored (e.g., through direct API calls to /properties/resolve), this is an XSS vector. The template literal ${p.address_line}, ${p.city} is injected directly into the DOM via .innerHTML. Should use textContent on individual elements or DOM API methods instead.

3. resultTarget.textContent (line 95) is safe -- textContent does not parse HTML. Good.

4. Missing submitOnEnter action. The docs (docs/hotwire.md line 50) reference a submitOnEnter() method, but the controller does not implement it and the ERB template has no keydown->location#submitOnEnter action. The old inline JS had houseNumber.addEventListener('keydown', ...) for Enter key submission. This is a behavioral regression -- users can no longer press Enter to save.

Rails controller -- correctness bugs:

5. Redundant double-update in resolve action (lines 26-27). The find_or_create_by! block already sets client_name and special_notes on creation. But lines 26-27 then call property.update() separately. For new records, this means: create with client_name = params[:client_name].presence || "TBD", then immediately update again with the same value. For existing records, the intent is to update fields -- but special_notes gets overwritten unconditionally whenever params[:special_notes].present?, even if the existing value was more complete. This logic is confusing and causes unnecessary DB writes. Consider consolidating into a single update-after-find-or-create pattern.

6. service_ids not validated. params[:service_ids].map(&:to_i) (line 30) will accept any array of values. Non-numeric strings become 0, which may create PropertyService records pointing to service ID 0 (nonexistent). The foreign key constraint in PostgreSQL would catch this, but the error would be an unhandled 500. Should validate that all service IDs exist before assignment.

7. No error handling around find_or_create_by! (line 16). The bang method raises ActiveRecord::RecordInvalid on validation failure. With validates :client_name, presence: true on Property, if somehow client_name ends up nil (edge case: params[:client_name] is blank AND presence returns nil), the action will 500 with no JSON error response. Should rescue and return structured error JSON.

CSS -- good:

• Design tokens in :root with no hardcoded hex outside definitions. Clean.
• Spacing uses token variables consistently. No magic numbers.
• Component sections well-documented with comments mapping to ERB views.
• No Tailwind (per project preference).

BLOCKERS

1. XSS via .innerHTML in renderRecent() -- location_controller.js line 121.
This is unvalidated user input rendered into the DOM. Property data originates from user-submitted values and is injected via .innerHTML without sanitization. This meets the BLOCKER criteria: "Unvalidated user input (SQL injection, XSS, path traversal risk)."

Fix: Replace .innerHTML with DOM construction using textContent:

renderRecent() {
  this.recentListTarget.replaceChildren(
    ...this.recent.map(p => {
      const li = document.createElement("li")
      li.className = "recent-item"
      li.textContent = `${p.address_line}, ${p.city}`
      return li
    })
  )
}

NITS

1. skip_forgery_protection on resolve should be removed now that the Stimulus controller sends CSRF tokens. Not a BLOCKER since the bypass existed before this PR, but it should not persist.

2. Missing submitOnEnter method referenced in docs/hotwire.md. Either add the method + action binding, or remove the reference from the docs. Behavioral regression from the prior inline JS.

3. Double DB writes in resolve -- the property.update() calls on lines 26-27 are redundant for new records and could be consolidated. Consider:

   property.assign_attributes(
     client_name: params[:client_name].presence || property.client_name,
     special_notes: params[:special_notes].presence || property.special_notes
   )
   property.save!
   

4. Service.order(:name).each in the ERB template (line 32 of index.html.erb) queries the DB directly from the view. Should be loaded in the controller (@services = Service.order(:name)) and referenced as @services.each in the view. This is a standard Rails convention to keep views free of query logic.

5. service_ids validation -- consider validating that submitted IDs correspond to actual Service records before assignment, to avoid unhandled 500s from FK violations.

6. formSection target declared but never toggled. The old inline JS hid #number-section until GPS succeeded (display: none). The new ERB shows the form always. The formSection target is declared in the controller but never used -- dead code. Either remove it or restore the hide/show behavior.

SOP COMPLIANCE

• Branch named after issue -- branch is hotwire, not 3-hotwire or 3-install-hotwire. Does not follow {issue-number}-{kebab-case-purpose} convention.
• PR body follows template -- has Summary, Changes, Test Plan, Related sections.
• Related references plan slug -- no plan slug referenced (confirmed: no plan slug exists for this work).
• No secrets committed -- no credentials, .env files, or API keys in the diff.
• No unnecessary file changes -- all 11 changed files are relevant to Hotwire installation and inline code migration.
• Commit messages -- not individually visible in the diff, cannot fully assess.

PROCESS OBSERVATIONS

• Documentation quality is strong. docs/hotwire.md is a good reference for the Hotwire setup and conventions. Minor inaccuracy: references submitOnEnter() which does not exist in the controller.
• Test coverage gap. No test files are included in this PR. The spec/ directory exists and rspec-rails is in the Gemfile, but no request specs for the resolve action (which now accepts new parameters) and no system/integration tests. The Test Plan section lists manual checks only. For an early-stage app this is understandable, but the new service_ids, client_name, and special_notes parameters on resolve should have request specs covering: happy path, missing fields, invalid service IDs, and the find-or-create vs. update-existing paths.
• Change failure risk: moderate. The XSS vector in renderRecent() is the primary risk. The CSRF bypass is inherited, not new, but should be cleaned up.

VERDICT: NOT APPROVED

One BLOCKER: XSS via .innerHTML in renderRecent(). Fix the DOM injection to use textContent or DOM construction methods. The skip_forgery_protection removal and submitOnEnter regression are strong recommendations but not formal blockers per criteria.

## PR #4 Review ### DOMAIN REVIEW **Tech stack:** Rails 8.1.3, Propshaft, importmap-rails, Hotwire (Turbo + Stimulus), PostgreSQL, plain CSS with design tokens. **Hotwire/Stimulus patterns -- well done:** - Clean Stimulus target wiring; all 12 targets declared and used correctly. - `data-action="click->location#detect"` and `data-action="click->location#save"` follow canonical Stimulus action syntax. - CSS visibility toggled via `.is-visible` class rather than inline `style.display` -- correct separation of concerns. - CSRF token now read from the meta tag and sent with fetch -- an improvement over the prior inline JS which omitted it entirely. - `connect()` lifecycle hook used properly for localStorage hydration. - `eagerLoadControllersFrom` + `pin_all_from` is the standard Rails 8 importmap setup. Correct. **Hotwire/Stimulus patterns -- issues:** 1. **`skip_forgery_protection` still active on `resolve` (line 2 of `properties_controller.rb`).** The Stimulus controller now correctly sends `X-CSRF-Token`, which means CSRF protection *should* work. But `skip_forgery_protection only: [:resolve]` disables it for that exact action. This was a necessary workaround when the inline JS did not send the token, but now that the Stimulus controller sends it, this line should be removed. Leaving it means CSRF protection is still bypassed on the only mutating endpoint in the app. **This is a security concern** but not a BLOCKER per the criteria below since there is no new *unvalidated user input* introduced -- the bypass existed before this PR. 2. **`renderRecent()` uses `.innerHTML` with unsanitized property data (line 121 of `location_controller.js`).** `address_line` and `city` come from user input via the Nominatim API response. If a malicious value gets stored (e.g., through direct API calls to `/properties/resolve`), this is an XSS vector. The template literal `${p.address_line}, ${p.city}` is injected directly into the DOM via `.innerHTML`. Should use `textContent` on individual elements or DOM API methods instead. 3. **`resultTarget.textContent` (line 95) is safe** -- `textContent` does not parse HTML. Good. 4. **Missing `submitOnEnter` action.** The docs (`docs/hotwire.md` line 50) reference a `submitOnEnter()` method, but the controller does not implement it and the ERB template has no `keydown->location#submitOnEnter` action. The old inline JS had `houseNumber.addEventListener('keydown', ...)` for Enter key submission. This is a behavioral regression -- users can no longer press Enter to save. **Rails controller -- correctness bugs:** 5. **Redundant double-update in `resolve` action (lines 26-27).** The `find_or_create_by!` block already sets `client_name` and `special_notes` on creation. But lines 26-27 then call `property.update()` separately. For *new* records, this means: create with `client_name = params[:client_name].presence || "TBD"`, then immediately update again with the same value. For *existing* records, the intent is to update fields -- but `special_notes` gets overwritten unconditionally whenever `params[:special_notes].present?`, even if the existing value was more complete. This logic is confusing and causes unnecessary DB writes. Consider consolidating into a single update-after-find-or-create pattern. 6. **`service_ids` not validated.** `params[:service_ids].map(&:to_i)` (line 30) will accept any array of values. Non-numeric strings become `0`, which may create `PropertyService` records pointing to service ID 0 (nonexistent). The foreign key constraint in PostgreSQL would catch this, but the error would be an unhandled 500. Should validate that all service IDs exist before assignment. 7. **No error handling around `find_or_create_by!` (line 16).** The bang method raises `ActiveRecord::RecordInvalid` on validation failure. With `validates :client_name, presence: true` on `Property`, if somehow `client_name` ends up nil (edge case: `params[:client_name]` is blank AND `presence` returns nil), the action will 500 with no JSON error response. Should rescue and return structured error JSON. **CSS -- good:** - Design tokens in `:root` with no hardcoded hex outside definitions. Clean. - Spacing uses token variables consistently. No magic numbers. - Component sections well-documented with comments mapping to ERB views. - No Tailwind (per project preference). ### BLOCKERS **1. XSS via `.innerHTML` in `renderRecent()` -- `location_controller.js` line 121.** This is unvalidated user input rendered into the DOM. Property data originates from user-submitted values and is injected via `.innerHTML` without sanitization. This meets the BLOCKER criteria: "Unvalidated user input (SQL injection, XSS, path traversal risk)." **Fix:** Replace `.innerHTML` with DOM construction using `textContent`: ```js renderRecent() { this.recentListTarget.replaceChildren( ...this.recent.map(p => { const li = document.createElement("li") li.className = "recent-item" li.textContent = `${p.address_line}, ${p.city}` return li }) ) } ``` ### NITS 1. **`skip_forgery_protection` on `resolve` should be removed** now that the Stimulus controller sends CSRF tokens. Not a BLOCKER since the bypass existed before this PR, but it should not persist. 2. **Missing `submitOnEnter` method** referenced in `docs/hotwire.md`. Either add the method + action binding, or remove the reference from the docs. Behavioral regression from the prior inline JS. 3. **Double DB writes in `resolve`** -- the `property.update()` calls on lines 26-27 are redundant for new records and could be consolidated. Consider: ```ruby property.assign_attributes( client_name: params[:client_name].presence || property.client_name, special_notes: params[:special_notes].presence || property.special_notes ) property.save! ``` 4. **`Service.order(:name).each` in the ERB template (line 32 of `index.html.erb`)** queries the DB directly from the view. Should be loaded in the controller (`@services = Service.order(:name)`) and referenced as `@services.each` in the view. This is a standard Rails convention to keep views free of query logic. 5. **`service_ids` validation** -- consider validating that submitted IDs correspond to actual `Service` records before assignment, to avoid unhandled 500s from FK violations. 6. **`formSection` target declared but never toggled.** The old inline JS hid `#number-section` until GPS succeeded (`display: none`). The new ERB shows the form always. The `formSection` target is declared in the controller but never used -- dead code. Either remove it or restore the hide/show behavior. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `hotwire`, not `3-hotwire` or `3-install-hotwire`. Does not follow `{issue-number}-{kebab-case-purpose}` convention. - [x] PR body follows template -- has Summary, Changes, Test Plan, Related sections. - [ ] Related references plan slug -- no plan slug referenced (confirmed: no plan slug exists for this work). - [x] No secrets committed -- no credentials, .env files, or API keys in the diff. - [x] No unnecessary file changes -- all 11 changed files are relevant to Hotwire installation and inline code migration. - [x] Commit messages -- not individually visible in the diff, cannot fully assess. ### PROCESS OBSERVATIONS - **Documentation quality is strong.** `docs/hotwire.md` is a good reference for the Hotwire setup and conventions. Minor inaccuracy: references `submitOnEnter()` which does not exist in the controller. - **Test coverage gap.** No test files are included in this PR. The `spec/` directory exists and `rspec-rails` is in the Gemfile, but no request specs for the `resolve` action (which now accepts new parameters) and no system/integration tests. The Test Plan section lists manual checks only. For an early-stage app this is understandable, but the new `service_ids`, `client_name`, and `special_notes` parameters on `resolve` should have request specs covering: happy path, missing fields, invalid service IDs, and the find-or-create vs. update-existing paths. - **Change failure risk: moderate.** The XSS vector in `renderRecent()` is the primary risk. The CSRF bypass is inherited, not new, but should be cleaned up. ### VERDICT: NOT APPROVED One BLOCKER: XSS via `.innerHTML` in `renderRecent()`. Fix the DOM injection to use `textContent` or DOM construction methods. The `skip_forgery_protection` removal and `submitOnEnter` regression are strong recommendations but not formal blockers per criteria.

ldraney added 1 commit 2026-05-24 18:24:00 +00:00

Fix XSS in renderRecent, remove skip_forgery_protection, clean up resolve ... 0d3a152ca7

- Replace innerHTML with DOM construction (textContent) to prevent XSS
- Remove skip_forgery_protection now that Stimulus sends CSRF tokens
- Consolidate redundant double-update in resolve action
- Move Service query from view to controller instance variable
- Restore submitOnEnter for house number field

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-05-24 22:28:48 +00:00

Fix resolve action: set client_name in create block to pass validation ... 45b03415e8

find_or_create_by! needs client_name in the block for new records since
the Property model validates its presence. The previous refactor moved
client_name to a separate update call that never ran because the bang
method raised first.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-05-24 22:31:19 +00:00

Add Accept: application/json header to resolve fetch request ... 815962164a

Without the Accept header, Turbo intercepts non-JSON responses (like
CSRF error pages) and the catch handler fires. Explicit Accept header
ensures Rails returns JSON and Turbo stays out of the way.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney merged commit ae50e63ea2 into main 2026-05-24 22:43:17 +00:00

ldraney deleted branch hotwire 2026-05-24 22:43:17 +00:00

ldraney referenced this pull request from a commit 2026-05-24 22:43:18 +00:00

Install Hotwire, migrate inline code to Stimulus + CSS tokens (#4)

ldraney referenced this pull request 2026-06-06 20:58:55 +00:00

Spike #121: Document client request flow (packages, projects, Stripe) #127

ldraney referenced this pull request 2026-06-07 17:16:15 +00:00

docs: auth strategy spike (Auth Code + PKCE + Keycloak themes + ASWebAuthenticationSession) #162

ldraney referenced this pull request 2026-06-07 17:36:01 +00:00

docs: auth strategy spike (Auth Code + PKCE + Keycloak themes + ASWebAuthenticationSession) #162

ldraney referenced this pull request from a commit 2026-06-08 04:15:58 +00:00

docs: update keycloak-setup.md with login theme architecture

ldraney referenced this pull request 2026-06-08 04:16:14 +00:00

docs: update keycloak-setup.md with login theme architecture #175

ldraney referenced this pull request 2026-06-08 04:16:30 +00:00

docs: update keycloak-setup.md with login theme architecture #175

ldraney referenced this pull request 2026-06-08 04:18:37 +00:00

docs: update keycloak-setup.md with login theme architecture #175

ldraney referenced this pull request 2026-06-16 03:10:14 +00:00

[DECOMPOSED] Day detail page: planning view with "Previously [Day]s" accordion #233

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!4

No description provided.