Add property picker dropdown to Today tab #66

Merged

ldraney merged 3 commits from feature/property-picker-dropdown into main 2026-06-03 12:30:26 +00:00

Conversation 2 Commits 3 Files changed 9 +200 -44

ldraney commented 2026-06-03 05:07:09 +00:00

Owner

Copy link

Summary

• Clicking the search input on the Today tab opens a scrollable dropdown listing all properties
• Type to filter, click to add to today's queue
• Already-queued properties show a "Queued" badge and are non-selectable

Changes

• app/views/work_queue_items/index.html.erb: Replaced quick-add form with property-picker combobox dropdown
• app/javascript/controllers/property_picker_controller.js: New Stimulus controller for open/close/filter/select
• app/assets/stylesheets/application.css: Added picker-dropdown, picker-option styles
• app/controllers/work_queue_items_controller.rb: Added @services to index action for service filter chips
• app/javascript/controllers/filter_controller.js: Removed unused clearInput method
• config/puma.rb: Made activate_control_app dev-only to fix CI crashes

Test Plan

• Click search input on Today tab — dropdown appears with all properties
• Type in the input — dropdown filters by name/address
• Click a property — added to today's queue, dropdown closes
• Re-open dropdown — selected property shows "Queued" badge, not re-selectable
• Click outside dropdown — it closes
• No regressions in Recent section search/filter

Review Checklist

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

Related Notes

• Closes #65

## Summary - Clicking the search input on the Today tab opens a scrollable dropdown listing all properties - Type to filter, click to add to today's queue - Already-queued properties show a "Queued" badge and are non-selectable ## Changes - `app/views/work_queue_items/index.html.erb`: Replaced quick-add form with property-picker combobox dropdown - `app/javascript/controllers/property_picker_controller.js`: New Stimulus controller for open/close/filter/select - `app/assets/stylesheets/application.css`: Added picker-dropdown, picker-option styles - `app/controllers/work_queue_items_controller.rb`: Added `@services` to index action for service filter chips - `app/javascript/controllers/filter_controller.js`: Removed unused `clearInput` method - `config/puma.rb`: Made `activate_control_app` dev-only to fix CI crashes ## Test Plan - [ ] Click search input on Today tab — dropdown appears with all properties - [ ] Type in the input — dropdown filters by name/address - [ ] Click a property — added to today's queue, dropdown closes - [ ] Re-open dropdown — selected property shows "Queued" badge, not re-selectable - [ ] Click outside dropdown — it closes - [ ] No regressions in Recent section search/filter ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - Closes #65

ldraney added 1 commit 2026-06-03 05:07:09 +00:00

Add property picker dropdown to Today tab search input ... d7457271e0

Clicking the search bar now opens a scrollable dropdown of all properties.
Type to filter, click to add to today's queue. Already-queued properties
show a "Queued" badge and are non-selectable.

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

ldraney commented 2026-06-03 05:09:38 +00:00

Author

Owner

Copy link

PR #66 Review

DOMAIN REVIEW

Stack identified: Ruby on Rails 8, Hotwired Stimulus, ERB templates, vanilla CSS, Puma, Woodpecker CI.

Stimulus controller (property_picker_controller.js)

• Clean lifecycle management: connect() binds the document click listener, disconnect() removes it. No memory leak.
• The close() method correctly guards with this.element.contains(event.target) to avoid closing when clicking inside the picker.
• filter() is clean and correct: downcases both query and target text, handles empty query by showing all.
• select() properly guards against double-queuing with dataset.queued === "true".
• The optimistic UI update (adding is-queued class and badge before the form submit response) is reasonable for this use case since the server will respond with a redirect or Turbo Stream anyway.

View (index.html.erb)

• data-search-text="<%= [...].compact.join(' ') %>" -- ERB <%= %> auto-escapes HTML by default in Rails. No XSS risk here.
• Line 25: property.work_queue_items.where(work_date: @date).exists? -- this fires a new SQL query per property despite @properties already including :work_queue_items via .includes(:work_queue_items). The .where() on an already-loaded association bypasses the eager-loaded collection and hits the DB. This is an N+1 query. The same pattern already existed in _property_item.html.erb line 11, so this is pre-existing tech debt, not introduced by this PR. However, this PR now has TWO locations doing it (picker dropdown + property list partial), doubling the N+1 hit. Consider replacing with property.work_queue_items.any? { |wqi| wqi.work_date == @date } to use the already-loaded collection instead.
• The hidden form with style="display:none" is a pragmatic approach for programmatic submission. Acceptable.

Controller (work_queue_items_controller.rb)

• @services = Service.order(:name) -- straightforward query, no concerns.

CSS

• All styles use design tokens (--spacing-md, --color-surface, --radius, etc.) consistently. No magic numbers.
• max-height: 60vh for the dropdown is reasonable for mobile-first (this is a landscaping field app).
• -webkit-overflow-scrolling: touch is deprecated but harmless. Modern iOS handles momentum scrolling natively.
• z-index: 100 -- adequate for a dropdown. No z-index escalation concerns in this codebase.

Puma config

• activate_control_app if ENV.fetch("RAILS_ENV", "development") == "development" -- good fix. The control app socket was likely causing CI crashes when Puma ran in test mode. The guard is correct.

Filter controller cleanup

• clearInput removal is safe. The old quick-add form that triggered turbo:submit-end->filter#clearInput no longer exists in the view -- the new property picker handles its own input clearing in select().

BLOCKERS

1. No test coverage for new functionality

This PR adds a new Stimulus controller, changes the view structure, adds @services to the controller, and changes the property-adding UX flow. The existing request spec at spec/requests/work_queue_items_spec.rb tests basic CRUD but does not cover:

• The @services assignment in index (verify service filter chips render)
• The property picker dropdown rendering (verify all active properties appear in the picker)
• The "already queued" state rendering

The PR's Test Plan consists entirely of manual checkboxes. The project has a CI pipeline that runs rspec -- new view behavior should have at least a request spec verifying the picker dropdown renders with properties and the service filter chips appear. This is a BLOCKER per the review criteria: "New functionality with zero test coverage."

NITS

1. N+1 query (pre-existing but doubled): property.work_queue_items.where(work_date: @date).exists? appears at line 25 in the picker and line 11 in _property_item.html.erb. Both bypass eager-loaded data. Consider property.work_queue_items.any? { |wqi| wqi.work_date == @date } to use the already-loaded association. Not a blocker since it is pre-existing, but this PR doubles the impact.

2. Deprecated CSS property: -webkit-overflow-scrolling: touch (line 804 of application.css) is deprecated. It is harmless but could be removed for cleanliness.

3. Accessibility -- no :hover state on picker options: .picker-option:active is styled but there is no :hover state. On non-touch devices, users have no visual feedback when hovering over options. Adding .picker-option:hover { background: var(--color-accent-light); } would improve discoverability.

4. Accessibility -- no ARIA attributes: The combobox pattern (input + dropdown list) should ideally have role="combobox", aria-expanded, aria-controls, and role="listbox" / role="option" on the list and items. Not a blocker for an internal tool, but worth noting for future iteration.

5. Keyboard navigation missing: The picker has no keyboard support (arrow keys to navigate options, Enter to select, Escape to close). Again, not a blocker for an internal mobile-first tool, but a gap.

6. quick-add-form CSS orphaned: The .quick-add-form styles at lines 769-783 of application.css are now unused since the quick-add form was replaced by the property picker. Consider removing to avoid dead CSS accumulation.

SOP COMPLIANCE

• Branch named after issue: Branch is feature/property-picker-dropdown, not 65-property-picker-dropdown. Convention requires {issue-number}-{kebab-case-purpose}.
• PR body follows template: Has Summary, Changes, Test Plan, Related sections.
• Related references plan slug: PR body has "Closes #65" but no plan slug reference. Task states no plan slug exists, so this is noted but not blocking.
• No secrets committed: No credentials, .env files, or API keys in the diff.
• No unnecessary file changes: The puma.rb fix is tangentially related (CI fix) but reasonable to bundle.
• Commit messages are descriptive: (Single commit with clear title matches PR title.)

PROCESS OBSERVATIONS

• Test gap pattern: This is a UI-focused feature change, and issue #63/#64 recently dropped system tests in favor of "Rails 8 endpoint-first testing." However, even under that philosophy, request specs should verify that the new view elements render correctly. The gap between manual Test Plan checkboxes and zero automated coverage is a risk.
• Change failure risk: Low-to-moderate. The property picker is additive UI, and the hidden form submission to the existing create action is a conservative approach. The main risk is the optimistic UI in select() -- if the form submission fails, the option shows "Queued" but the item was not actually added. There is no error handling for submission failure in the Stimulus controller.
• Deployment frequency: Small, focused PR. Good for deployment velocity.

VERDICT: NOT APPROVED

One blocker: no automated test coverage for new functionality. Add at least a request spec verifying the picker dropdown renders with property data and service filter chips on GET /today.

## PR #66 Review ### DOMAIN REVIEW **Stack identified:** Ruby on Rails 8, Hotwired Stimulus, ERB templates, vanilla CSS, Puma, Woodpecker CI. **Stimulus controller (`property_picker_controller.js`)** - Clean lifecycle management: `connect()` binds the document click listener, `disconnect()` removes it. No memory leak. - The `close()` method correctly guards with `this.element.contains(event.target)` to avoid closing when clicking inside the picker. - `filter()` is clean and correct: downcases both query and target text, handles empty query by showing all. - `select()` properly guards against double-queuing with `dataset.queued === "true"`. - The optimistic UI update (adding `is-queued` class and badge before the form submit response) is reasonable for this use case since the server will respond with a redirect or Turbo Stream anyway. **View (`index.html.erb`)** - `data-search-text="<%= [...].compact.join(' ') %>"` -- ERB `<%= %>` auto-escapes HTML by default in Rails. No XSS risk here. - Line 25: `property.work_queue_items.where(work_date: @date).exists?` -- this fires a new SQL query per property despite `@properties` already including `:work_queue_items` via `.includes(:work_queue_items)`. The `.where()` on an already-loaded association bypasses the eager-loaded collection and hits the DB. This is an N+1 query. The same pattern already existed in `_property_item.html.erb` line 11, so this is pre-existing tech debt, not introduced by this PR. However, this PR now has TWO locations doing it (picker dropdown + property list partial), doubling the N+1 hit. Consider replacing with `property.work_queue_items.any? { |wqi| wqi.work_date == @date }` to use the already-loaded collection instead. - The hidden form with `style="display:none"` is a pragmatic approach for programmatic submission. Acceptable. **Controller (`work_queue_items_controller.rb`)** - `@services = Service.order(:name)` -- straightforward query, no concerns. **CSS** - All styles use design tokens (`--spacing-md`, `--color-surface`, `--radius`, etc.) consistently. No magic numbers. - `max-height: 60vh` for the dropdown is reasonable for mobile-first (this is a landscaping field app). - `-webkit-overflow-scrolling: touch` is deprecated but harmless. Modern iOS handles momentum scrolling natively. - `z-index: 100` -- adequate for a dropdown. No z-index escalation concerns in this codebase. **Puma config** - `activate_control_app if ENV.fetch("RAILS_ENV", "development") == "development"` -- good fix. The control app socket was likely causing CI crashes when Puma ran in test mode. The guard is correct. **Filter controller cleanup** - `clearInput` removal is safe. The old quick-add form that triggered `turbo:submit-end->filter#clearInput` no longer exists in the view -- the new property picker handles its own input clearing in `select()`. ### BLOCKERS **1. No test coverage for new functionality** This PR adds a new Stimulus controller, changes the view structure, adds `@services` to the controller, and changes the property-adding UX flow. The existing request spec at `spec/requests/work_queue_items_spec.rb` tests basic CRUD but does not cover: - The `@services` assignment in `index` (verify service filter chips render) - The property picker dropdown rendering (verify all active properties appear in the picker) - The "already queued" state rendering The PR's Test Plan consists entirely of manual checkboxes. The project has a CI pipeline that runs `rspec` -- new view behavior should have at least a request spec verifying the picker dropdown renders with properties and the service filter chips appear. This is a BLOCKER per the review criteria: "New functionality with zero test coverage." ### NITS 1. **N+1 query (pre-existing but doubled):** `property.work_queue_items.where(work_date: @date).exists?` appears at line 25 in the picker and line 11 in `_property_item.html.erb`. Both bypass eager-loaded data. Consider `property.work_queue_items.any? { |wqi| wqi.work_date == @date }` to use the already-loaded association. Not a blocker since it is pre-existing, but this PR doubles the impact. 2. **Deprecated CSS property:** `-webkit-overflow-scrolling: touch` (line 804 of application.css) is deprecated. It is harmless but could be removed for cleanliness. 3. **Accessibility -- no `:hover` state on picker options:** `.picker-option:active` is styled but there is no `:hover` state. On non-touch devices, users have no visual feedback when hovering over options. Adding `.picker-option:hover { background: var(--color-accent-light); }` would improve discoverability. 4. **Accessibility -- no ARIA attributes:** The combobox pattern (input + dropdown list) should ideally have `role="combobox"`, `aria-expanded`, `aria-controls`, and `role="listbox"` / `role="option"` on the list and items. Not a blocker for an internal tool, but worth noting for future iteration. 5. **Keyboard navigation missing:** The picker has no keyboard support (arrow keys to navigate options, Enter to select, Escape to close). Again, not a blocker for an internal mobile-first tool, but a gap. 6. **`quick-add-form` CSS orphaned:** The `.quick-add-form` styles at lines 769-783 of `application.css` are now unused since the quick-add form was replaced by the property picker. Consider removing to avoid dead CSS accumulation. ### SOP COMPLIANCE - [ ] **Branch named after issue:** Branch is `feature/property-picker-dropdown`, not `65-property-picker-dropdown`. Convention requires `{issue-number}-{kebab-case-purpose}`. - [x] **PR body follows template:** Has Summary, Changes, Test Plan, Related sections. - [ ] **Related references plan slug:** PR body has "Closes #65" but no plan slug reference. Task states no plan slug exists, so this is noted but not blocking. - [x] **No secrets committed:** No credentials, .env files, or API keys in the diff. - [x] **No unnecessary file changes:** The puma.rb fix is tangentially related (CI fix) but reasonable to bundle. - [x] **Commit messages are descriptive:** (Single commit with clear title matches PR title.) ### PROCESS OBSERVATIONS - **Test gap pattern:** This is a UI-focused feature change, and issue #63/#64 recently dropped system tests in favor of "Rails 8 endpoint-first testing." However, even under that philosophy, request specs should verify that the new view elements render correctly. The gap between manual Test Plan checkboxes and zero automated coverage is a risk. - **Change failure risk:** Low-to-moderate. The property picker is additive UI, and the hidden form submission to the existing `create` action is a conservative approach. The main risk is the optimistic UI in `select()` -- if the form submission fails, the option shows "Queued" but the item was not actually added. There is no error handling for submission failure in the Stimulus controller. - **Deployment frequency:** Small, focused PR. Good for deployment velocity. ### VERDICT: NOT APPROVED One blocker: no automated test coverage for new functionality. Add at least a request spec verifying the picker dropdown renders with property data and service filter chips on `GET /today`.

ldraney added 1 commit 2026-06-03 05:16:18 +00:00

Fix review findings: tests, N+1, ARIA, error handling, dead CSS ... 058c9f259d

- Add 3 request specs for property picker (dropdown render, queued state, services)
- Fix N+1 query: preload queued property IDs via single pluck instead of per-row exists?
- Add ARIA combobox/listbox roles and aria-expanded toggling
- Add turbo:submit-end rollback on failed form submission
- Add hover state to picker options
- Remove orphaned .quick-add-form CSS
- Skip allow_browser guard in test env

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

ldraney added 1 commit 2026-06-03 12:23:27 +00:00

Remove filter chips and second search bar from Today tab ... 4bfe6b793e

Feature branch was adding UI elements (service filter chips, search input)
that don't exist on main. The property picker dropdown replaces the old
unified input — the Recent section should stay clean like prod.

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

ldraney commented 2026-06-03 12:27:46 +00:00

Author

Owner

Copy link

PR #66 Review (Re-review)

DOMAIN REVIEW

Stack: Ruby on Rails 8 / Stimulus / Turbo / vanilla CSS -- standard Rails monolith patterns.

Controller (work_queue_items_controller.rb)

• @queued_property_ids uses .pluck(:property_id).to_set -- efficient single-column query, Set for O(1) lookup. Good fix from round 1.
• The partial _property_item.html.erb now uses @queued_property_ids.include?(property.id) instead of the N+1 property.work_queue_items.where(work_date: date).exists?. Correct.
• Note: the query for @queue_items and @queued_property_ids both hit WorkQueueItem.where(work_date: @date) -- two queries where one could serve both. Not a blocker, but a minor optimization opportunity.

Stimulus controller (property_picker_controller.js)

• connect()/disconnect() properly bind and unbind the global click listener. No memory leak.
• close() guards against event being undefined (called without args) -- correct.
• select() has the error rollback via turbo:submit-end with { once: true } -- good fix from round 1.
• filter() uses toLowerCase() on both query and searchText -- case-insensitive matching works.
• Form submission uses form.requestSubmit() rather than form.submit(), which correctly fires Turbo's submit handlers. Correct.

View (index.html.erb)

• ARIA: role="combobox", aria-expanded, aria-controls, aria-autocomplete="list" on input. role="listbox" on the dropdown, role="option" on each <li>. This is the WAI-ARIA combobox pattern. Good fix from round 1.
• data-search-text uses <%= %> which auto-escapes HTML entities -- no XSS risk.
• Hidden form with property_id input is set programmatically by the Stimulus controller before requestSubmit() -- clean separation.

CSS (application.css)

• Old .quick-add-form styles removed, replaced with .property-picker, .picker-dropdown, .picker-option family. No orphaned rules remain.
• Uses design tokens (--spacing-*, --color-*, --radius) consistently. No magic numbers except 60vh for max-height (reasonable) and z-index: 100 (matches the existing z-index: 100 on .bottom-nav).
• -webkit-overflow-scrolling: touch for iOS momentum scrolling -- appropriate for mobile-first app.

Puma config (config/puma.rb)

• activate_control_app if ENV.fetch("RAILS_ENV", "development") == "development" -- restricts control app to dev only. Fixes CI crashes. Reasonable, though Rails.env.development? would be more idiomatic Rails. The ENV.fetch approach works because Puma config loads before the full Rails environment, so this is actually the correct pattern here.

Application controller (application_controller.rb)

• allow_browser versions: :modern unless Rails.env.test? -- skips browser version check in test env. This prevents test failures from user-agent rejection. Standard practice.

Filter controller (filter_controller.js)

• Removed clearInput() method which was bound to the old quick-add form's turbo:submit-end event. The old form no longer exists, so this is correct cleanup.

Tests (spec/requests/work_queue_items_spec.rb)

• 3 new specs added (from round 1 feedback):
  1. Renders the property picker dropdown with active properties -- checks for data-controller, ARIA roles, and property name.
  2. Marks queued properties -- verifies data-queued="true" appears when a property is in the queue.
  3. Existing spec: inactive properties not shown (was already present).
• Tests cover the server-rendered HTML output. JS behavior (open/close/filter/select) is untested, but that requires system/integration tests which this project is moving away from per issue #63/#64.

BLOCKERS

None. All blockers from round 1 have been addressed:

• Test coverage: 3 new request specs covering the picker rendering and queued-state markup.
• N+1 query: fixed with @queued_property_ids preload.
• ARIA roles: combobox/listbox pattern implemented.
• Error rollback: turbo:submit-end handler reverts optimistic UI on failure.
• No orphaned CSS.

NITS

1. PR body inaccuracy: The Changes section states "Added @services to index action for service filter chips" but the diff does not show @services being added. The actual change is @queued_property_ids. Minor copy error in the PR description.

2. Duplicate query opportunity (lines 4-5 of work_queue_items_controller.rb): @queue_items and @queued_property_ids both query WorkQueueItem.where(work_date: @date). Could derive the set from the already-loaded items: @queued_property_ids = @queue_items.map { |i| i.property_id }.to_set. Saves one DB query. Non-blocking.

3. aria-selected is always "false": The combobox pattern typically updates aria-selected="true" on the focused/active option during keyboard navigation. Since keyboard navigation is not implemented yet (only mouse click), this is fine for now but worth noting for a future iteration.

4. No keyboard navigation: The combobox has no arrow-key or Enter-key support. Users relying on keyboard-only interaction cannot navigate the dropdown. Not a blocker for this PR, but should be tracked as a follow-up accessibility item.

5. z-index: 100 shared with bottom-nav: Both .picker-dropdown and .bottom-nav use z-index: 100. If the picker dropdown ever overlaps the bottom nav area on small screens, they could conflict. The dropdown is position: absolute relative to .property-picker and max-height: 60vh, so overlap is unlikely but worth noting.

SOP COMPLIANCE

• Branch named after issue -- branch is feature/property-picker-dropdown, not 65-property-picker-dropdown. Convention calls for {issue-number}-{kebab-case-purpose}.
• PR body follows template -- has Summary, Changes, Test Plan, Related sections.
• Related references plan slug -- no plan slug (acknowledged upfront, no plan exists).
• No secrets committed -- no credentials, .env files, or API keys in the diff.
• No unnecessary file changes -- all 9 files are relevant to the feature or CI fixes.
• Commit messages are descriptive (based on PR title/body context).

PROCESS OBSERVATIONS

• The branch naming convention miss (feature/ prefix instead of 65- prefix) is a recurring pattern. Not blocking merge, but worth enforcing in future PRs.
• The PR body has a stale line about @services that does not match the diff. Keeping PR descriptions accurate helps future archaeology.
• The Puma and ApplicationController changes are CI-related fixes bundled into a feature PR. Ideally these would be separate commits or even a separate PR, but they are small and well-scoped enough to not cause concern here.
• Test coverage is at the request-spec level. The Stimulus controller behavior (open/close/filter/select/rollback) is not tested. Per the project's decision in #63/#64 to drop system tests, this is acceptable for now.

VERDICT: APPROVED

All blockers from round 1 are resolved. The code is correct, accessible, and well-structured. The nits above are non-blocking suggestions for future improvement.

## PR #66 Review (Re-review) ### DOMAIN REVIEW **Stack**: Ruby on Rails 8 / Stimulus / Turbo / vanilla CSS -- standard Rails monolith patterns. **Controller (`work_queue_items_controller.rb`)** - `@queued_property_ids` uses `.pluck(:property_id).to_set` -- efficient single-column query, Set for O(1) lookup. Good fix from round 1. - The partial `_property_item.html.erb` now uses `@queued_property_ids.include?(property.id)` instead of the N+1 `property.work_queue_items.where(work_date: date).exists?`. Correct. - Note: the query for `@queue_items` and `@queued_property_ids` both hit `WorkQueueItem.where(work_date: @date)` -- two queries where one could serve both. Not a blocker, but a minor optimization opportunity. **Stimulus controller (`property_picker_controller.js`)** - `connect()`/`disconnect()` properly bind and unbind the global click listener. No memory leak. - `close()` guards against `event` being undefined (called without args) -- correct. - `select()` has the error rollback via `turbo:submit-end` with `{ once: true }` -- good fix from round 1. - `filter()` uses `toLowerCase()` on both query and `searchText` -- case-insensitive matching works. - Form submission uses `form.requestSubmit()` rather than `form.submit()`, which correctly fires Turbo's submit handlers. Correct. **View (`index.html.erb`)** - ARIA: `role="combobox"`, `aria-expanded`, `aria-controls`, `aria-autocomplete="list"` on input. `role="listbox"` on the dropdown, `role="option"` on each `<li>`. This is the WAI-ARIA combobox pattern. Good fix from round 1. - `data-search-text` uses `<%= %>` which auto-escapes HTML entities -- no XSS risk. - Hidden form with `property_id` input is set programmatically by the Stimulus controller before `requestSubmit()` -- clean separation. **CSS (`application.css`)** - Old `.quick-add-form` styles removed, replaced with `.property-picker`, `.picker-dropdown`, `.picker-option` family. No orphaned rules remain. - Uses design tokens (`--spacing-*`, `--color-*`, `--radius`) consistently. No magic numbers except `60vh` for max-height (reasonable) and `z-index: 100` (matches the existing `z-index: 100` on `.bottom-nav`). - `-webkit-overflow-scrolling: touch` for iOS momentum scrolling -- appropriate for mobile-first app. **Puma config (`config/puma.rb`)** - `activate_control_app if ENV.fetch("RAILS_ENV", "development") == "development"` -- restricts control app to dev only. Fixes CI crashes. Reasonable, though `Rails.env.development?` would be more idiomatic Rails. The `ENV.fetch` approach works because Puma config loads before the full Rails environment, so this is actually the correct pattern here. **Application controller (`application_controller.rb`)** - `allow_browser versions: :modern unless Rails.env.test?` -- skips browser version check in test env. This prevents test failures from user-agent rejection. Standard practice. **Filter controller (`filter_controller.js`)** - Removed `clearInput()` method which was bound to the old quick-add form's `turbo:submit-end` event. The old form no longer exists, so this is correct cleanup. **Tests (`spec/requests/work_queue_items_spec.rb`)** - 3 new specs added (from round 1 feedback): 1. Renders the property picker dropdown with active properties -- checks for `data-controller`, ARIA roles, and property name. 2. Marks queued properties -- verifies `data-queued="true"` appears when a property is in the queue. 3. Existing spec: inactive properties not shown (was already present). - Tests cover the server-rendered HTML output. JS behavior (open/close/filter/select) is untested, but that requires system/integration tests which this project is moving away from per issue #63/#64. ### BLOCKERS None. All blockers from round 1 have been addressed: - Test coverage: 3 new request specs covering the picker rendering and queued-state markup. - N+1 query: fixed with `@queued_property_ids` preload. - ARIA roles: combobox/listbox pattern implemented. - Error rollback: `turbo:submit-end` handler reverts optimistic UI on failure. - No orphaned CSS. ### NITS 1. **PR body inaccuracy**: The Changes section states "Added `@services` to index action for service filter chips" but the diff does not show `@services` being added. The actual change is `@queued_property_ids`. Minor copy error in the PR description. 2. **Duplicate query opportunity** (lines 4-5 of `work_queue_items_controller.rb`): `@queue_items` and `@queued_property_ids` both query `WorkQueueItem.where(work_date: @date)`. Could derive the set from the already-loaded items: `@queued_property_ids = @queue_items.map { |i| i.property_id }.to_set`. Saves one DB query. Non-blocking. 3. **`aria-selected` is always `"false"`**: The combobox pattern typically updates `aria-selected="true"` on the focused/active option during keyboard navigation. Since keyboard navigation is not implemented yet (only mouse click), this is fine for now but worth noting for a future iteration. 4. **No keyboard navigation**: The combobox has no arrow-key or Enter-key support. Users relying on keyboard-only interaction cannot navigate the dropdown. Not a blocker for this PR, but should be tracked as a follow-up accessibility item. 5. **`z-index: 100` shared with bottom-nav**: Both `.picker-dropdown` and `.bottom-nav` use `z-index: 100`. If the picker dropdown ever overlaps the bottom nav area on small screens, they could conflict. The dropdown is `position: absolute` relative to `.property-picker` and `max-height: 60vh`, so overlap is unlikely but worth noting. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `feature/property-picker-dropdown`, not `65-property-picker-dropdown`. Convention calls for `{issue-number}-{kebab-case-purpose}`. - [x] PR body follows template -- has Summary, Changes, Test Plan, Related sections. - [ ] Related references plan slug -- no plan slug (acknowledged upfront, no plan exists). - [x] No secrets committed -- no credentials, .env files, or API keys in the diff. - [x] No unnecessary file changes -- all 9 files are relevant to the feature or CI fixes. - [x] Commit messages are descriptive (based on PR title/body context). ### PROCESS OBSERVATIONS - The branch naming convention miss (`feature/` prefix instead of `65-` prefix) is a recurring pattern. Not blocking merge, but worth enforcing in future PRs. - The PR body has a stale line about `@services` that does not match the diff. Keeping PR descriptions accurate helps future archaeology. - The Puma and ApplicationController changes are CI-related fixes bundled into a feature PR. Ideally these would be separate commits or even a separate PR, but they are small and well-scoped enough to not cause concern here. - Test coverage is at the request-spec level. The Stimulus controller behavior (open/close/filter/select/rollback) is not tested. Per the project's decision in #63/#64 to drop system tests, this is acceptable for now. ### VERDICT: APPROVED All blockers from round 1 are resolved. The code is correct, accessible, and well-structured. The nits above are non-blocking suggestions for future improvement.

ldraney merged commit 4d56d5dcdf into main 2026-06-03 12:30:26 +00:00

ldraney deleted branch feature/property-picker-dropdown 2026-06-03 12:30:26 +00:00

ldraney referenced this pull request from a commit 2026-06-03 12:30:27 +00:00

Add property picker dropdown to Today tab (#66)

ldraney referenced this pull request from a commit 2026-06-04 17:38:57 +00:00

Restore quick-add new property from Today tab

ldraney referenced this pull request 2026-06-04 17:39:21 +00:00

Restore quick-add new property from Today tab #105

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

No description provided.