Add nav, work queue, and fix CSRF/mobile polish #6

Merged

ldraney merged 3 commits from feature/nav-and-work-queue into main 2026-05-25 01:32:11 +00:00

Conversation 1 Commits 3 Files changed 21 +585 -27

ldraney commented 2026-05-25 01:22:34 +00:00

Owner

Copy link

Summary

• Bottom nav (New / Today tabs), daily work queue with day navigation, search, and service filters
• Complete/uncomplete toggle and add/remove via Turbo Streams
• Fix CSRF origin check failure through Tailscale funnel proxy
• Mobile touch target polish and safe-area support

Changes

• app/controllers/work_queue_items_controller.rb: CRUD actions with Turbo Stream responses
• app/views/work_queue_items/: Index, partials, and Turbo Stream templates for create/update/destroy
• app/views/layouts/application.html.erb: Bottom nav, viewport-fit=cover for safe-area
• app/assets/stylesheets/application.css: Queue, day-nav, filter chip, and bottom-nav styles; 44px touch targets; safe-area padding
• app/javascript/controllers/filter_controller.js: Search and service filter Stimulus controller
• config/environments/development.rb: Disable CSRF origin check for Tailscale funnel
• config/routes.rb: work_queue_items resource
• Rakefile: Fix stale dev port (9999 → 3000)

Test Plan

• Complete toggle: checkmark toggles strikethrough + DB state, round-trip works
• Remove: X removes from queue, property flips back to add button
• Add: + on property adds to queue, shows "Queued" badge
• CSRF: all Turbo Stream actions work through Tailscale funnel without 422
• Mobile: 375px viewport, touch targets adequate, no overflow

Review Checklist

• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive

Related Notes

• Closes #5

## Summary - Bottom nav (New / Today tabs), daily work queue with day navigation, search, and service filters - Complete/uncomplete toggle and add/remove via Turbo Streams - Fix CSRF origin check failure through Tailscale funnel proxy - Mobile touch target polish and safe-area support ## Changes - `app/controllers/work_queue_items_controller.rb`: CRUD actions with Turbo Stream responses - `app/views/work_queue_items/`: Index, partials, and Turbo Stream templates for create/update/destroy - `app/views/layouts/application.html.erb`: Bottom nav, viewport-fit=cover for safe-area - `app/assets/stylesheets/application.css`: Queue, day-nav, filter chip, and bottom-nav styles; 44px touch targets; safe-area padding - `app/javascript/controllers/filter_controller.js`: Search and service filter Stimulus controller - `config/environments/development.rb`: Disable CSRF origin check for Tailscale funnel - `config/routes.rb`: work_queue_items resource - `Rakefile`: Fix stale dev port (9999 → 3000) ## Test Plan - [x] Complete toggle: checkmark toggles strikethrough + DB state, round-trip works - [x] Remove: X removes from queue, property flips back to add button - [x] Add: + on property adds to queue, shows "Queued" badge - [x] CSRF: all Turbo Stream actions work through Tailscale funnel without 422 - [x] Mobile: 375px viewport, touch targets adequate, no overflow ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #5

ldraney added 2 commits 2026-05-25 01:22:34 +00:00

Add navigation, work queue, search, and service filters ... cdfc891a01

WIP for #5. Two-tab bottom nav (New/Today), WorkQueueItem model
with day-scoped queues, Turbo Stream responses for reactive
toggle/add/remove, Stimulus-powered search and service filter
chips. Strikethrough on completed items needs further work.

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

Fix CSRF through Tailscale funnel, mobile touch target polish ... 6e76747804

Disable origin-vs-base_url CSRF check in development — the funnel
proxies with a different Origin than Rails sees internally. Token
validation still runs. Also bumps touch targets to 44px, adds
safe-area padding for notched phones, and fixes stale Rakefile port.

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

ldraney commented 2026-05-25 01:24:42 +00:00

Author

Owner

Copy link

PR #6 Review

DOMAIN REVIEW

Stack: Rails 8.1, Hotwire (Turbo Streams + Stimulus), vanilla CSS, PostgreSQL

Rails / Controller

1. Date.parse without rescue (work_queue_items_controller.rb, lines 3 and 14): Date.parse(params[:date]) will raise Date::Error on malformed input (e.g. ?date=garbage). This is user-supplied input arriving via query string. Rails will turn this into a 500. Wrap in a begin/rescue or use a safer parse pattern.

2. N+1 on _property_item.html.erb line 8: property.work_queue_items.where(work_date: date).exists? fires a query per property. The @properties query in index does includes(:services) but does not include :work_queue_items, so this is an N+1. At scale this will degrade noticeably. Consider preloading or building a Set of queued property IDs in the controller.

3. update action does not check .update return value (line 30): @item.update(completed: !@item.completed) -- if the update fails for any reason, the controller still renders the Turbo Stream response as if it succeeded. Should handle the failure case.

4. create position race condition (line 15): WorkQueueItem.where(work_date: ...).count is not atomic. Two concurrent creates for the same date could get the same position value. Not critical for a single-user app, but worth noting.

Migration

5. Missing composite index: The model validates uniqueness: { scope: :work_date } on property_id, and the controller queries where(work_date: @date).order(:position). The migration only creates an index on property_id (via t.references). A composite index on [:work_date, :property_id] (unique) would enforce the uniqueness constraint at the DB level and optimize the most common query pattern. Currently the uniqueness validation is only enforced at the application level, which is insufficient under concurrent requests.

Turbo Streams

6. Turbo Stream templates are correctly structured. create.turbo_stream.erb appends the queue item, replaces the property item to show "Queued" badge, and removes the empty state. destroy.turbo_stream.erb removes the item and replaces the property to restore the add button. update.turbo_stream.erb replaces the item in-place for the completion toggle. All target IDs match (dom_id(item), property_{id}, queue-list, queue-empty). This is clean Hotwire.

CSS / Mobile

7. CSS uses design tokens consistently. Touch targets are 2.75rem (44px) which meets the WCAG minimum. env(safe-area-inset-bottom) and viewport-fit=cover correctly handle iPhone notch/home-bar. page-content padding-bottom of 5rem clears the fixed bottom nav. No Tailwind (as required by project preferences).

8. One hardcoded color: .btn-remove:hover { background: #fef2f2; } (line 400 in CSS). All other colors use tokens. Consider adding a --color-danger-light token for consistency.

Stimulus

9. filter_controller.js is clean. Targets are properly declared, search is case-insensitive, service filter toggling works via class manipulation. No DOM injection vulnerabilities -- it only reads dataset attributes and toggles display.

CSRF

10. forgery_protection_origin_check = false in development.rb is acceptable for the stated use case (Tailscale funnel proxy with origin mismatch). The comment explains the rationale. CSRF token validation still runs. This is dev-only -- production config is unaffected.

XSS Surface

11. _property_item.html.erb line 2: data-search-text="<%= [property.address_line, ...].compact.join(' ') %>" -- ERB's <%= %> auto-escapes by default in Rails, so user content in data attributes is safe here. The output is used as a data attribute value (read by JS for string matching), not rendered as HTML.

BLOCKERS

B1: Test coverage is effectively zero. spec/models/work_queue_item_spec.rb contains only a pending placeholder -- no actual tests. This is a new resource with a full CRUD controller, model validations, uniqueness constraint, and Turbo Stream responses. Per BLOCKER criteria: "New functionality with zero test coverage." At minimum, needs:

• Model specs: validation presence, uniqueness scope, scopes (for_today, pending)
• Request specs: create/update/destroy with both HTML and Turbo Stream formats, including the error path (duplicate create)

B2: Unvalidated user input on params[:date]. Date.parse(params[:date]) in both index and create actions will raise an unhandled exception on malformed input. This is a 500 error reachable by any user typing a bad URL. Needs a rescue or validation.

NITS

N1. Hardcoded color: .btn-remove:hover uses #fef2f2 instead of a design token. Add --color-danger-light: #fef2f2 to :root.

N2. Empty WorkQueue comment block (CSS line 264-266): There is a Component: WorkQueue section heading comment with no styles between it and the DayNav comment. Either remove the empty comment or add a note that the component styles follow below.

N3. properties#index route duplication: Routes now have both resources :properties, only: [:index, ...] and root "properties#index". Both generate GET /properties and GET /. This is intentional (root is the homepage) but the explicit :index in the resources was added in this PR -- verify it is needed for something beyond the root route.

N4. Nav active state: current_page?(work_queue_items_path) will not match when a ?date= param is present (e.g. navigating to a different day). The "Today" tab will lose its active highlight when viewing any date other than the default. Consider using controller_name == "work_queue_items" instead.

SOP COMPLIANCE

• Branch named after issue -- Branch is feature/nav-and-work-queue, not 5-nav-and-work-queue. Does not follow {issue-number}-{kebab-case-purpose} convention.
• PR body follows template -- Has Summary, Changes, Test Plan, Related sections.
• Related references plan slug -- PR body says "Closes #5" but no plan slug referenced (noted: no plan slug exists for this work).
• No secrets committed
• No unnecessary file changes -- Rakefile port fix, networking docs update, and dev hostname change are all related to the work.
• Commit messages are descriptive -- Single commit cdfc891 with clear message.

PROCESS OBSERVATIONS

• This is a well-structured feature PR with clean Hotwire patterns. The Turbo Stream create/update/destroy cycle is correctly wired with matching DOM IDs across partials and stream templates.
• The N+1 query in the property list (finding if each property is already queued) will become a problem as the property count grows. Worth addressing before it becomes a user-visible performance issue.
• The branch naming miss is minor but worth establishing the convention early in the project.
• The test gap is the primary concern. This is the first controller with mutable state beyond the initial property resolver -- the CRUD surface needs coverage.

VERDICT: NOT APPROVED

Two blockers must be resolved:

1. Add real test coverage for the WorkQueueItem model and controller (model specs + request specs).
2. Handle malformed params[:date] input gracefully (rescue Date::Error, fall back to Date.current or return 400).

## PR #6 Review ### DOMAIN REVIEW **Stack:** Rails 8.1, Hotwire (Turbo Streams + Stimulus), vanilla CSS, PostgreSQL **Rails / Controller** 1. **`Date.parse` without rescue** (`work_queue_items_controller.rb`, lines 3 and 14): `Date.parse(params[:date])` will raise `Date::Error` on malformed input (e.g. `?date=garbage`). This is user-supplied input arriving via query string. Rails will turn this into a 500. Wrap in a begin/rescue or use a safer parse pattern. 2. **N+1 on `_property_item.html.erb` line 8**: `property.work_queue_items.where(work_date: date).exists?` fires a query per property. The `@properties` query in `index` does `includes(:services)` but does not include `:work_queue_items`, so this is an N+1. At scale this will degrade noticeably. Consider preloading or building a `Set` of queued property IDs in the controller. 3. **`update` action does not check `.update` return value** (line 30): `@item.update(completed: !@item.completed)` -- if the update fails for any reason, the controller still renders the Turbo Stream response as if it succeeded. Should handle the failure case. 4. **`create` position race condition** (line 15): `WorkQueueItem.where(work_date: ...).count` is not atomic. Two concurrent creates for the same date could get the same position value. Not critical for a single-user app, but worth noting. **Migration** 5. **Missing composite index**: The model validates `uniqueness: { scope: :work_date }` on `property_id`, and the controller queries `where(work_date: @date).order(:position)`. The migration only creates an index on `property_id` (via `t.references`). A composite index on `[:work_date, :property_id]` (unique) would enforce the uniqueness constraint at the DB level and optimize the most common query pattern. Currently the uniqueness validation is only enforced at the application level, which is insufficient under concurrent requests. **Turbo Streams** 6. Turbo Stream templates are correctly structured. `create.turbo_stream.erb` appends the queue item, replaces the property item to show "Queued" badge, and removes the empty state. `destroy.turbo_stream.erb` removes the item and replaces the property to restore the add button. `update.turbo_stream.erb` replaces the item in-place for the completion toggle. All target IDs match (`dom_id(item)`, `property_{id}`, `queue-list`, `queue-empty`). This is clean Hotwire. **CSS / Mobile** 7. CSS uses design tokens consistently. Touch targets are 2.75rem (44px) which meets the WCAG minimum. `env(safe-area-inset-bottom)` and `viewport-fit=cover` correctly handle iPhone notch/home-bar. `page-content` padding-bottom of 5rem clears the fixed bottom nav. No Tailwind (as required by project preferences). 8. One hardcoded color: `.btn-remove:hover { background: #fef2f2; }` (line 400 in CSS). All other colors use tokens. Consider adding a `--color-danger-light` token for consistency. **Stimulus** 9. `filter_controller.js` is clean. Targets are properly declared, search is case-insensitive, service filter toggling works via class manipulation. No DOM injection vulnerabilities -- it only reads `dataset` attributes and toggles `display`. **CSRF** 10. `forgery_protection_origin_check = false` in `development.rb` is acceptable for the stated use case (Tailscale funnel proxy with origin mismatch). The comment explains the rationale. CSRF token validation still runs. This is dev-only -- production config is unaffected. **XSS Surface** 11. `_property_item.html.erb` line 2: `data-search-text="<%= [property.address_line, ...].compact.join(' ') %>"` -- ERB's `<%= %>` auto-escapes by default in Rails, so user content in data attributes is safe here. The output is used as a data attribute value (read by JS for string matching), not rendered as HTML. ### BLOCKERS **B1: Test coverage is effectively zero.** `spec/models/work_queue_item_spec.rb` contains only a `pending` placeholder -- no actual tests. This is a new resource with a full CRUD controller, model validations, uniqueness constraint, and Turbo Stream responses. Per BLOCKER criteria: "New functionality with zero test coverage." At minimum, needs: - Model specs: validation presence, uniqueness scope, scopes (`for_today`, `pending`) - Request specs: create/update/destroy with both HTML and Turbo Stream formats, including the error path (duplicate create) **B2: Unvalidated user input on `params[:date]`.** `Date.parse(params[:date])` in both `index` and `create` actions will raise an unhandled exception on malformed input. This is a 500 error reachable by any user typing a bad URL. Needs a rescue or validation. ### NITS N1. **Hardcoded color**: `.btn-remove:hover` uses `#fef2f2` instead of a design token. Add `--color-danger-light: #fef2f2` to `:root`. N2. **Empty WorkQueue comment block** (CSS line 264-266): There is a `Component: WorkQueue` section heading comment with no styles between it and the `DayNav` comment. Either remove the empty comment or add a note that the component styles follow below. N3. **`properties#index` route duplication**: Routes now have both `resources :properties, only: [:index, ...]` and `root "properties#index"`. Both generate `GET /properties` and `GET /`. This is intentional (root is the homepage) but the explicit `:index` in the resources was added in this PR -- verify it is needed for something beyond the root route. N4. **Nav active state**: `current_page?(work_queue_items_path)` will not match when a `?date=` param is present (e.g. navigating to a different day). The "Today" tab will lose its active highlight when viewing any date other than the default. Consider using `controller_name == "work_queue_items"` instead. ### SOP COMPLIANCE - [ ] Branch named after issue -- Branch is `feature/nav-and-work-queue`, not `5-nav-and-work-queue`. 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 -- PR body says "Closes #5" but no plan slug referenced (noted: no plan slug exists for this work). - [x] No secrets committed - [x] No unnecessary file changes -- Rakefile port fix, networking docs update, and dev hostname change are all related to the work. - [x] Commit messages are descriptive -- Single commit `cdfc891` with clear message. ### PROCESS OBSERVATIONS - This is a well-structured feature PR with clean Hotwire patterns. The Turbo Stream create/update/destroy cycle is correctly wired with matching DOM IDs across partials and stream templates. - The N+1 query in the property list (finding if each property is already queued) will become a problem as the property count grows. Worth addressing before it becomes a user-visible performance issue. - The branch naming miss is minor but worth establishing the convention early in the project. - The test gap is the primary concern. This is the first controller with mutable state beyond the initial property resolver -- the CRUD surface needs coverage. ### VERDICT: NOT APPROVED Two blockers must be resolved: 1. Add real test coverage for the WorkQueueItem model and controller (model specs + request specs). 2. Handle malformed `params[:date]` input gracefully (rescue `Date::Error`, fall back to `Date.current` or return 400).

ldraney added 1 commit 2026-05-25 01:26:54 +00:00

Fix QA blockers: date validation, nav state, specs, composite index ... b720ca612c

Rescue Date::Error on malformed date params instead of 500. Fix nav
active state to use controller_name instead of current_page? (breaks
with query params). Add composite unique index on [work_date, property_id].
Add model + request specs (15 passing). Eager-load work_queue_items
to fix N+1 in property list.

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

ldraney merged commit f020fb7b0f into main 2026-05-25 01:32:11 +00:00

ldraney deleted branch feature/nav-and-work-queue 2026-05-25 01:32:12 +00:00

ldraney referenced this pull request from a commit 2026-05-25 01:32:13 +00:00

Add nav, work queue, and fix CSRF/mobile polish (#6)

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-06 22:31:16 +00:00

Spike: Keycloak programmatic configuration and architecture validation #133

ldraney referenced this pull request from a commit 2026-06-06 22:32:25 +00:00

Fix QA blockers: add NetworkPolicy step, remove stale SOP follow-up

ldraney referenced this pull request 2026-06-06 22:35:13 +00:00

Spike: Keycloak programmatic configuration and architecture validation #133

ldraney referenced this pull request 2026-06-06 22:53:35 +00:00

Spike: Keycloak programmatic configuration and architecture validation #133

ldraney referenced this pull request 2026-06-08 05:11:47 +00:00

Client request UI: My Property view with package and request form #123

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!6

No description provided.