UI polish: property detail, edit, filters, week queued indicator

ldraney commented

2026-05-25 02:54:08 +00:00

Author

Owner

PR #14 Review (Post-Merge)

Scope: UI polish -- property detail/edit pages, week view queued indicator, service filter chips, recents links, case-insensitive sort.
Tech stack: Ruby on Rails, Hotwire (Stimulus + Turbo), vanilla CSS with design tokens.
Files changed: 10 files, +196/-19 lines. Closes #8.

DOMAIN REVIEW

Rails / Controller correctness

Arel.sql("LOWER(client_name)") in properties_controller.rb:7 -- This is safe. The string is a static literal, not interpolated from user input. Arel.sql is the correct way to pass raw SQL fragments for ordering, and Rails requires it explicitly to prevent accidental injection. No risk here.
properties_controller.rb edit action properly loads @services for the form. The update action correctly re-loads @services in the failure path (format.html block) so the edit form re-renders with checkboxes. Good pattern.
property_params now permits service_ids: [] -- correct for array checkbox submission.
respond_to block in update correctly handles both HTML (form submission from edit page) and JSON (existing API usage from Stimulus save). Backward-compatible change.
weeks_controller.rb refactor from a single completions query to a split @completions/@queued hash is clean. Single query, partitioned in Ruby. No N+1.
routes.rb correctly adds :edit to the only: list. RESTful.

Turbo Stream / manage.html.erb consistency

The turbo stream template (toggle_active.turbo_stream.erb) and manage.html.erb now use identical markup:

Both use link_to property_path(property) with class: "manage-item-info"
Both show client_name as <strong>, address in manage-item-meta
Both include data-search-text and data-service-ids attributes
Structure is consistent. No divergence risk.

CSS quality

New styles (.form-row, .form-field-sm, .property-details, .property-location, .recent-item a, .recent-item-meta, .week-queued) all use design tokens for colors (--color-muted, --color-accent) and spacing (--spacing-sm, --spacing-md, --spacing-lg, --spacing-xs).
Component sections are clearly labeled with comment headers referencing the view file (good).
gap: 0.15rem on .recent-item a (line 234) is a raw value rather than a spacing token. Minor -- there is no token for sub---spacing-xs gaps, so this is defensible.
font-size: 0.85rem on .recent-item-meta (line 240) is a raw value. No font-size tokens exist in the token set, so this matches the existing pattern elsewhere in the file (e.g., .form-field label uses 0.9rem).
flex: 0 0 5rem on .form-field-sm (line 149) -- raw value for a fixed-width field. Reasonable for state/zip inputs.

ERB templates

show.html.erb and edit.html.erb both use ERB output tags (<%= %>), which auto-escape HTML by default. No XSS risk in server-rendered views.
edit.html.erb uses form_with model: which auto-generates CSRF token. Correct.
edit.html.erb labels use hardcoded for="property_address_line" etc. -- these match Rails' default id generation for f.text_field :address_line, so they are correct.

BLOCKERS

1. XSS via innerHTML in location_controller.js:132 -- MEDIUM SEVERITY

a.innerHTML = `<strong>${p.address_line}</strong> <span class="recent-item-meta">${p.client_name || ""} &middot; ${p.city}</span>`

The data source (p) comes from localStorage("recent_properties"), which is populated from the JSON response of /properties/resolve (line 94-98). The flow:

User enters address/client name via the form
Server creates/finds property, returns JSON (no server-side HTML escaping in JSON)
Client stores raw property object in localStorage
renderRecent() interpolates p.address_line, p.client_name, p.city directly into innerHTML

Attack vector: If a property's address_line or client_name contains <script> or <img onerror=...>, it will execute. While the immediate risk is self-XSS (the user entering the data is the same user seeing it), localStorage persists across sessions and could be poisoned by another XSS vector or shared-device scenario.

Fix: Use textContent on individual elements instead of innerHTML template interpolation:

const a = document.createElement("a")
a.href = `/properties/${p.id}`
const strong = document.createElement("strong")
strong.textContent = p.address_line
const meta = document.createElement("span")
meta.className = "recent-item-meta"
meta.textContent = `${p.client_name || ""} · ${p.city}`
a.appendChild(strong)
a.append(" ")
a.appendChild(meta)

Note: The previous version of this code used li.textContent (safe). This PR regressed to innerHTML.

2. No test coverage for new functionality

This PR adds:

edit action on PropertiesController
Modified update action (now respond_to with HTML/JSON)
Modified weeks_controller.rb (queued hash logic)
Service filter chips on manage page
Recents linking behavior

None of these have controller tests, integration tests, or system tests. The test plan in the PR body is manual-only (checkboxes). Per BLOCKER criteria: "New functionality with zero test coverage."

This is a landscaping side project and the entire codebase appears to have no test suite, so this is flagged for tracking rather than as a gate -- but it is technically a BLOCKER per the review criteria.

NITS

weeks_controller.rb:8 -- @properties = Property.active.order(:address_line) still sorts by address_line while the manage page now sorts by LOWER(client_name). Consider whether the week view should also sort by client name for consistency, since the manage page now leads with client name as the primary identifier.
show.html.erb -- No back/navigation link. The user lands here from the manage list or recents, but there is no way to go back except the browser back button (or the bottom nav, if it is rendered outside this template). Consider adding a back link.
edit.html.erb -- No cancel button. If the user navigates to edit and changes their mind, they must use browser back. A "Cancel" link back to property_path(@property) would improve UX.
show.html.erb -- Missing @property.services eager loading. The show action does Property.find(params[:id]), then the template calls @property.services.any? and @property.services.map. This triggers a lazy-load query. Low impact for a single property, but adding .includes(:services) to the show action would be cleaner.
edit.html.erb -- Same lazy-load observation for @property.service_ids.include? in the checkbox loop. The edit action does not eager-load services on the property.
location_controller.js:131 -- a.href = \/properties/${p.id}`-- thep.idvalue comes from localStorage. If corrupted, this could produce an invalid URL. Low risk but worth noting. Consider validatingp.id` is a positive integer.

SOP COMPLIANCE

Branch named after issue: PR was from refs/pull/14/head. Cannot confirm branch name format from merged PR metadata. Closes #8, so branch should have been 8-*.
PR body follows template: Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections.
Related references plan slug: Related section says "Closes #8" but does not reference a plan slug from pal-e-docs.
No secrets committed: No credentials, .env files, or API keys in the diff.
No scope creep: All changes are UI polish aligned with the issue description.
Commit messages: Single-commit PR with descriptive title.

PROCESS OBSERVATIONS

Test debt accumulating: This is the 6th merged PR with no automated tests. As the app grows, manual testing becomes unreliable. A single system test covering the create-view-edit flow would catch regressions in the property lifecycle.
Turbo stream / manage template sync: The manual discipline of keeping these in sync is working now, but a shared partial would eliminate the risk of future divergence. Consider extracting _manage_item.html.erb.
Change failure risk: Low. The changes are additive (new views, new CSS) with one behavioral change (update now responds to HTML). The respond_to block preserves the existing JSON path, so the Stimulus save flow is unaffected.

VERDICT: NOT APPROVED

Two blockers:

innerHTML XSS regression in location_controller.js -- the previous code was safe (textContent), this PR introduced innerHTML interpolation of user-controlled data. Should be fixed.
Zero test coverage for new controller actions and modified behavior. Flagged per criteria, acknowledging this is a side project with no existing test suite.

## PR #14 Review (Post-Merge) **Scope**: UI polish -- property detail/edit pages, week view queued indicator, service filter chips, recents links, case-insensitive sort. **Tech stack**: Ruby on Rails, Hotwire (Stimulus + Turbo), vanilla CSS with design tokens. **Files changed**: 10 files, +196/-19 lines. Closes #8. --- ### DOMAIN REVIEW **Rails / Controller correctness** 1. `Arel.sql("LOWER(client_name)")` in `properties_controller.rb:7` -- This is safe. The string is a static literal, not interpolated from user input. `Arel.sql` is the correct way to pass raw SQL fragments for ordering, and Rails requires it explicitly to prevent accidental injection. No risk here. 2. `properties_controller.rb` `edit` action properly loads `@services` for the form. The `update` action correctly re-loads `@services` in the failure path (`format.html` block) so the edit form re-renders with checkboxes. Good pattern. 3. `property_params` now permits `service_ids: []` -- correct for array checkbox submission. 4. `respond_to` block in `update` correctly handles both HTML (form submission from edit page) and JSON (existing API usage from Stimulus save). Backward-compatible change. 5. `weeks_controller.rb` refactor from a single completions query to a split `@completions`/`@queued` hash is clean. Single query, partitioned in Ruby. No N+1. 6. `routes.rb` correctly adds `:edit` to the `only:` list. RESTful. **Turbo Stream / manage.html.erb consistency** The turbo stream template (`toggle_active.turbo_stream.erb`) and `manage.html.erb` now use identical markup: - Both use `link_to property_path(property)` with `class: "manage-item-info"` - Both show `client_name` as `<strong>`, address in `manage-item-meta` - Both include `data-search-text` and `data-service-ids` attributes - Structure is consistent. No divergence risk. **CSS quality** - New styles (`.form-row`, `.form-field-sm`, `.property-details`, `.property-location`, `.recent-item a`, `.recent-item-meta`, `.week-queued`) all use design tokens for colors (`--color-muted`, `--color-accent`) and spacing (`--spacing-sm`, `--spacing-md`, `--spacing-lg`, `--spacing-xs`). - Component sections are clearly labeled with comment headers referencing the view file (good). - `gap: 0.15rem` on `.recent-item a` (line 234) is a raw value rather than a spacing token. Minor -- there is no token for sub-`--spacing-xs` gaps, so this is defensible. - `font-size: 0.85rem` on `.recent-item-meta` (line 240) is a raw value. No font-size tokens exist in the token set, so this matches the existing pattern elsewhere in the file (e.g., `.form-field label` uses `0.9rem`). - `flex: 0 0 5rem` on `.form-field-sm` (line 149) -- raw value for a fixed-width field. Reasonable for state/zip inputs. **ERB templates** - `show.html.erb` and `edit.html.erb` both use ERB output tags (`<%= %>`), which auto-escape HTML by default. No XSS risk in server-rendered views. - `edit.html.erb` uses `form_with model:` which auto-generates CSRF token. Correct. - `edit.html.erb` labels use hardcoded `for="property_address_line"` etc. -- these match Rails' default `id` generation for `f.text_field :address_line`, so they are correct. --- ### BLOCKERS **1. XSS via innerHTML in `location_controller.js:132` -- MEDIUM SEVERITY** ```js a.innerHTML = `<strong>${p.address_line}</strong> <span class="recent-item-meta">${p.client_name || ""} · ${p.city}</span>` ``` The data source (`p`) comes from `localStorage("recent_properties")`, which is populated from the JSON response of `/properties/resolve` (line 94-98). The flow: 1. User enters address/client name via the form 2. Server creates/finds property, returns JSON (no server-side HTML escaping in JSON) 3. Client stores raw property object in localStorage 4. `renderRecent()` interpolates `p.address_line`, `p.client_name`, `p.city` directly into `innerHTML` **Attack vector**: If a property's `address_line` or `client_name` contains `<script>` or `<img onerror=...>`, it will execute. While the immediate risk is self-XSS (the user entering the data is the same user seeing it), localStorage persists across sessions and could be poisoned by another XSS vector or shared-device scenario. **Fix**: Use `textContent` on individual elements instead of `innerHTML` template interpolation: ```js const a = document.createElement("a") a.href = `/properties/${p.id}` const strong = document.createElement("strong") strong.textContent = p.address_line const meta = document.createElement("span") meta.className = "recent-item-meta" meta.textContent = `${p.client_name || ""} · ${p.city}` a.appendChild(strong) a.append(" ") a.appendChild(meta) ``` Note: The previous version of this code used `li.textContent` (safe). This PR regressed to `innerHTML`. **2. No test coverage for new functionality** This PR adds: - `edit` action on `PropertiesController` - Modified `update` action (now `respond_to` with HTML/JSON) - Modified `weeks_controller.rb` (queued hash logic) - Service filter chips on manage page - Recents linking behavior None of these have controller tests, integration tests, or system tests. The test plan in the PR body is manual-only (checkboxes). Per BLOCKER criteria: "New functionality with zero test coverage." This is a landscaping side project and the entire codebase appears to have no test suite, so this is flagged for tracking rather than as a gate -- but it is technically a BLOCKER per the review criteria. --- ### NITS 1. **`weeks_controller.rb:8`** -- `@properties = Property.active.order(:address_line)` still sorts by `address_line` while the manage page now sorts by `LOWER(client_name)`. Consider whether the week view should also sort by client name for consistency, since the manage page now leads with client name as the primary identifier. 2. **`show.html.erb`** -- No back/navigation link. The user lands here from the manage list or recents, but there is no way to go back except the browser back button (or the bottom nav, if it is rendered outside this template). Consider adding a back link. 3. **`edit.html.erb`** -- No cancel button. If the user navigates to edit and changes their mind, they must use browser back. A "Cancel" link back to `property_path(@property)` would improve UX. 4. **`show.html.erb`** -- Missing `@property.services` eager loading. The `show` action does `Property.find(params[:id])`, then the template calls `@property.services.any?` and `@property.services.map`. This triggers a lazy-load query. Low impact for a single property, but adding `.includes(:services)` to the `show` action would be cleaner. 5. **`edit.html.erb`** -- Same lazy-load observation for `@property.service_ids.include?` in the checkbox loop. The `edit` action does not eager-load services on the property. 6. **`location_controller.js:131`** -- `a.href = \`/properties/${p.id}\`` -- the `p.id` value comes from localStorage. If corrupted, this could produce an invalid URL. Low risk but worth noting. Consider validating `p.id` is a positive integer. --- ### SOP COMPLIANCE - [ ] **Branch named after issue**: PR was from `refs/pull/14/head`. Cannot confirm branch name format from merged PR metadata. Closes #8, so branch should have been `8-*`. - [x] **PR body follows template**: Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections. - [ ] **Related references plan slug**: Related section says "Closes #8" but does not reference a plan slug from pal-e-docs. - [x] **No secrets committed**: No credentials, .env files, or API keys in the diff. - [x] **No scope creep**: All changes are UI polish aligned with the issue description. - [x] **Commit messages**: Single-commit PR with descriptive title. --- ### PROCESS OBSERVATIONS - **Test debt accumulating**: This is the 6th merged PR with no automated tests. As the app grows, manual testing becomes unreliable. A single system test covering the create-view-edit flow would catch regressions in the property lifecycle. - **Turbo stream / manage template sync**: The manual discipline of keeping these in sync is working now, but a shared partial would eliminate the risk of future divergence. Consider extracting `_manage_item.html.erb`. - **Change failure risk**: Low. The changes are additive (new views, new CSS) with one behavioral change (update now responds to HTML). The `respond_to` block preserves the existing JSON path, so the Stimulus save flow is unaffected. --- ### VERDICT: NOT APPROVED Two blockers: 1. **innerHTML XSS regression** in `location_controller.js` -- the previous code was safe (`textContent`), this PR introduced `innerHTML` interpolation of user-controlled data. Should be fixed. 2. **Zero test coverage** for new controller actions and modified behavior. Flagged per criteria, acknowledging this is a side project with no existing test suite.

2026-05-25 02:51:35 +00:00

Summary

UI polish pass: week view now shows queued and completed items, property detail/edit pages, service filters on Properties tab, case-insensitive name sort.

Changes

Week view shows both queued (dot) and completed (checkmark) items under correct day columns
Property detail page (show) with readable layout and Edit button
Edit page with full form: address, client, city/state/zip, services, notes
Recents on New tab show client name and link to property detail
Service filter chips added to Properties tab (matching Today tab)
Properties sorted by client name (case-insensitive), name displayed as primary/bold text

Test Plan

Add property to today queue, check week view shows dot under correct day
Complete property in queue, check week view shows checkmark
Click property in Properties tab, verify detail page renders
Click Edit on detail page, update fields, save
Save property on New tab, verify recents show name and link
Filter properties by service chip on Properties tab

Review Checklist

No Tailwind
Turbo Stream templates match manage.html.erb structure
CSS uses design tokens

Closes #8

🤖 Generated with Claude Code

## Summary UI polish pass: week view now shows queued and completed items, property detail/edit pages, service filters on Properties tab, case-insensitive name sort. ## Changes - Week view shows both queued (dot) and completed (checkmark) items under correct day columns - Property detail page (show) with readable layout and Edit button - Edit page with full form: address, client, city/state/zip, services, notes - Recents on New tab show client name and link to property detail - Service filter chips added to Properties tab (matching Today tab) - Properties sorted by client name (case-insensitive), name displayed as primary/bold text ## Test Plan - [ ] Add property to today queue, check week view shows dot under correct day - [ ] Complete property in queue, check week view shows checkmark - [ ] Click property in Properties tab, verify detail page renders - [ ] Click Edit on detail page, update fields, save - [ ] Save property on New tab, verify recents show name and link - [ ] Filter properties by service chip on Properties tab ## Review Checklist - [x] No Tailwind - [x] Turbo Stream templates match manage.html.erb structure - [x] CSS uses design tokens ## Related Notes Closes #8 🤖 Generated with [Claude Code](https://claude.com/claude-code)

ldraney added 2 commits

Show queued items in weekly view alongside completions

ci/woodpecker/push/woodpecker Pipeline was successful

Details

09b36902c1

The week grid now displays both completed (checkmark) and queued-but-incomplete
(dot) work queue items under their respective day columns.

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

Add property detail/edit pages, service filters, and UI polish

ci/woodpecker/pr/woodpecker Pipeline was successful

CI / scan_ruby (pull_request) Has been cancelled

CI / scan_js (pull_request) Has been cancelled

CI / lint (pull_request) Has been cancelled

4ced8f5ad2

- Show page with readable property details, edit button links to form
- Edit page with address, client, city/state/zip, services, notes
- Recents on New tab show client name and link to property detail
- Service filter chips on Properties tab (same as Today tab)
- Properties sorted by client name (case-insensitive), name shown bold

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

ldraney merged commit b1f6bd8c1e into main

2026-05-25 02:51:41 +00:00

ldraney deleted branch iterate-polish

ldraney referenced this pull request from a commit

2026-05-25 02:51:42 +00:00

UI polish: property detail, edit, filters, week queued indicator (#14)

Rows
Columns

UI polish: property detail, edit, filters, week queued indicator #14