Restore quick-add new property from Today tab

ldraney commented

2026-06-04 17:40:48 +00:00

Author

Owner

PR #105 Review

DOMAIN REVIEW

Stack: Rails 8.1 / Hotwired (Stimulus + Turbo Streams) / RSpec request specs

Stimulus controller (property_picker_controller.js)

The submit() method is well-structured:

Guards on event.key !== "Enter" before doing anything -- correct.
event.preventDefault() stops the default form submission or newline -- correct.
Clears propertyIdTarget before setting clientNameTarget so the two params are mutually exclusive -- good.
Restores input text on failure via a { once: true } turbo:submit-end listener -- correct pattern.
The refactor from querySelector calls to Stimulus targets (formTarget, propertyIdTarget) in the select() method is a clean improvement that reduces fragile DOM queries.

No issues found in the JS.

View (index.html.erb)

The hidden client_name field is properly wired with the data-property-picker-target="clientName" attribute.
The keydown->property-picker#submit action binding on the input is correct.
The form target on the form_with tag is correct.
Both hidden fields default to empty strings, so only the one populated by JS will carry a value -- clean.

No issues found in the view.

Controller (work_queue_items_controller.rb, not in the diff but reviewed for context)

Property.create! with client_name: params[:client_name] uses bang create, so a validation failure (blank name) raises ActiveRecord::RecordInvalid. The Property model has validates :client_name, presence: true, which catches blank/nil. However, create! will raise an unrescued exception rather than returning a user-friendly turbo stream error. The JS submit() method trims and checks for empty before sending, so in practice this path should not be hit from the UI, but a direct POST with client_name: "" would 500. This is a pre-existing pattern in the controller (not introduced by this PR) and the JS front-gates it, so I am noting it as a nit rather than a blocker.
params[:client_name] goes through Rails strong-ish parameter handling. Since client_name is a simple string attribute with a presence validation, SQL injection risk is mitigated by ActiveRecord parameterized queries. No raw SQL is involved. Acceptable.

Request spec (work_queue_items_spec.rb)

The new test covers the happy path: creates a Property, creates a WorkQueueItem, verifies turbo stream response includes the client name and a prepend action. This confirms the @new_property branch in the turbo stream template works.
The test checks both .change(Property, :count).by(1) and .change(WorkQueueItem, :count).by(1) -- good compound assertion.

BLOCKERS

None found.

Test coverage: New functionality (Enter-to-create) has a request spec covering the server-side path. The JS behavior is inherently Stimulus wiring (not separately unit-tested in this project's test suite, which is request-spec-based). The Playwright verification in the test plan covers the E2E path. Acceptable.
Input validation: client_name is validated by validates :client_name, presence: true on the model. The JS trims whitespace and rejects empty strings before submitting. No unvalidated user input reaches the database.
Secrets: None committed.
DRY/auth: No duplicated auth logic. No auth paths affected.

NITS

Branch naming: Branch is fix-quick-add-from-today-tab but SOP convention is {issue-number}-{kebab-case-purpose}. Should be 98-restore-quick-add-from-today-tab or similar. This is a process nit; the PR body correctly references Closes #98.
Unrescued create! on blank client_name: If someone POSTs directly with client_name: " " (whitespace only), Property.create! will raise ActiveRecord::RecordInvalid and return a 500. The JS front-gates this, so it is not reachable from the UI, but switching to create (no bang) with an error branch would be more defensive. Pre-existing pattern, not introduced by this PR.
Missing edge case test -- blank client_name: A test like post "/today", params: { client_name: "", date: Date.current.to_s } to confirm the server handles it gracefully (even if it currently 500s) would document the expected behavior. Low priority since the JS prevents this path.
Missing edge case test -- duplicate client_name: There is no uniqueness constraint on client_name in the schema or model. A user could type "Test" and create a second property with the same name. This may be intentional (same client, different property), but a test documenting this behavior would be useful.
Related Notes section is empty: PR body has Related Notes: None. -- the SOP asks for a plan slug reference in the Related section. The Related section does reference Closes #98 which is correct, but there is no plan slug. Minor if no plan exists for this work.

SOP COMPLIANCE

Branch named after issue -- branch is fix-quick-add-from-today-tab, missing 98- prefix per convention
PR body follows template -- Summary, Changes, Test Plan, Related all present and well-written
Related references plan slug -- Closes #98 is present but no plan slug referenced
No secrets committed
No unnecessary file changes -- all three files are directly relevant to the feature
Tests exist and pass -- 20 examples, 0 failures per test plan
Commit messages are descriptive (PR title is clear)

PROCESS OBSERVATIONS

Change failure risk: Low. The change is additive (new Stimulus method, new hidden field, new test). The refactor of querySelector to Stimulus targets in select() is the only modification to existing behavior, and it is a mechanical improvement with identical semantics.
Deployment frequency: No impact. Standard feature PR.
Test plan quality: Above average -- includes RSpec, Playwright E2E verification, and regression checks on existing picker behavior.

VERDICT: APPROVED

The code is clean, well-scoped, and tested. The branch naming and missing plan slug are process nits that do not affect code quality or correctness. No blockers found.

## PR #105 Review ### DOMAIN REVIEW **Stack**: Rails 8.1 / Hotwired (Stimulus + Turbo Streams) / RSpec request specs **Stimulus controller (`property_picker_controller.js`)** The `submit()` method is well-structured: - Guards on `event.key !== "Enter"` before doing anything -- correct. - `event.preventDefault()` stops the default form submission or newline -- correct. - Clears `propertyIdTarget` before setting `clientNameTarget` so the two params are mutually exclusive -- good. - Restores input text on failure via a `{ once: true }` turbo:submit-end listener -- correct pattern. - The refactor from `querySelector` calls to Stimulus targets (`formTarget`, `propertyIdTarget`) in the `select()` method is a clean improvement that reduces fragile DOM queries. No issues found in the JS. **View (`index.html.erb`)** - The hidden `client_name` field is properly wired with the `data-property-picker-target="clientName"` attribute. - The `keydown->property-picker#submit` action binding on the input is correct. - The `form` target on the `form_with` tag is correct. - Both hidden fields default to empty strings, so only the one populated by JS will carry a value -- clean. No issues found in the view. **Controller (`work_queue_items_controller.rb`, not in the diff but reviewed for context)** - `Property.create!` with `client_name: params[:client_name]` uses bang create, so a validation failure (blank name) raises `ActiveRecord::RecordInvalid`. The `Property` model has `validates :client_name, presence: true`, which catches blank/nil. However, `create!` will raise an unrescued exception rather than returning a user-friendly turbo stream error. The JS `submit()` method trims and checks for empty before sending, so in practice this path should not be hit from the UI, but a direct POST with `client_name: ""` would 500. This is a pre-existing pattern in the controller (not introduced by this PR) and the JS front-gates it, so I am noting it as a nit rather than a blocker. - `params[:client_name]` goes through Rails strong-ish parameter handling. Since `client_name` is a simple string attribute with a presence validation, SQL injection risk is mitigated by ActiveRecord parameterized queries. No raw SQL is involved. Acceptable. **Request spec (`work_queue_items_spec.rb`)** - The new test covers the happy path: creates a Property, creates a WorkQueueItem, verifies turbo stream response includes the client name and a `prepend` action. This confirms the `@new_property` branch in the turbo stream template works. - The test checks both `.change(Property, :count).by(1)` and `.change(WorkQueueItem, :count).by(1)` -- good compound assertion. ### BLOCKERS None found. - **Test coverage**: New functionality (Enter-to-create) has a request spec covering the server-side path. The JS behavior is inherently Stimulus wiring (not separately unit-tested in this project's test suite, which is request-spec-based). The Playwright verification in the test plan covers the E2E path. Acceptable. - **Input validation**: `client_name` is validated by `validates :client_name, presence: true` on the model. The JS trims whitespace and rejects empty strings before submitting. No unvalidated user input reaches the database. - **Secrets**: None committed. - **DRY/auth**: No duplicated auth logic. No auth paths affected. ### NITS 1. **Branch naming**: Branch is `fix-quick-add-from-today-tab` but SOP convention is `{issue-number}-{kebab-case-purpose}`. Should be `98-restore-quick-add-from-today-tab` or similar. This is a process nit; the PR body correctly references `Closes #98`. 2. **Unrescued `create!` on blank `client_name`**: If someone POSTs directly with `client_name: " "` (whitespace only), `Property.create!` will raise `ActiveRecord::RecordInvalid` and return a 500. The JS front-gates this, so it is not reachable from the UI, but switching to `create` (no bang) with an error branch would be more defensive. Pre-existing pattern, not introduced by this PR. 3. **Missing edge case test -- blank `client_name`**: A test like `post "/today", params: { client_name: "", date: Date.current.to_s }` to confirm the server handles it gracefully (even if it currently 500s) would document the expected behavior. Low priority since the JS prevents this path. 4. **Missing edge case test -- duplicate `client_name`**: There is no uniqueness constraint on `client_name` in the schema or model. A user could type "Test" and create a second property with the same name. This may be intentional (same client, different property), but a test documenting this behavior would be useful. 5. **`Related Notes` section is empty**: PR body has `Related Notes: None.` -- the SOP asks for a plan slug reference in the Related section. The `Related` section does reference `Closes #98` which is correct, but there is no plan slug. Minor if no plan exists for this work. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `fix-quick-add-from-today-tab`, missing `98-` prefix per convention - [x] PR body follows template -- Summary, Changes, Test Plan, Related all present and well-written - [ ] Related references plan slug -- `Closes #98` is present but no plan slug referenced - [x] No secrets committed - [x] No unnecessary file changes -- all three files are directly relevant to the feature - [x] Tests exist and pass -- 20 examples, 0 failures per test plan - [x] Commit messages are descriptive (PR title is clear) ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The change is additive (new Stimulus method, new hidden field, new test). The refactor of `querySelector` to Stimulus targets in `select()` is the only modification to existing behavior, and it is a mechanical improvement with identical semantics. - **Deployment frequency**: No impact. Standard feature PR. - **Test plan quality**: Above average -- includes RSpec, Playwright E2E verification, and regression checks on existing picker behavior. ### VERDICT: APPROVED The code is clean, well-scoped, and tested. The branch naming and missing plan slug are process nits that do not affect code quality or correctness. No blockers found.

2026-06-04 17:39:21 +00:00

Summary

The property picker (PR #66) replaced the unified search+quick-add input but lost the Enter-to-create path for new properties
The controller already handled client_name param — this wires the UI back up
Type a name in the search box, hit Enter, property is created + queued in one action

Changes

app/views/work_queue_items/index.html.erb — Add hidden client_name field and Stimulus targets to the picker form, add keydown action binding
app/javascript/controllers/property_picker_controller.js — Add submit() Enter handler that sends client_name instead of property_id, add form/field targets to replace querySelector calls
spec/requests/work_queue_items_spec.rb — Add request spec covering the quick-add turbo stream path (20 examples, 0 failures)

Test Plan

docker compose run --rm -e RAILS_ENV=test web bundle exec rspec — 20 examples, 0 failures
Verified in Playwright on https://landscaping-dev.tail5b443a.ts.net/today — typed "New Test Client", pressed Enter, property created + queued + appeared in Recent with "Queued" badge
Existing property picker select (click to queue) still works
Existing add buttons in Recent and Last Thursday sections unaffected

Review Checklist

Quick-add creates property AND queues it in one action
Input clears after successful submit
Input restores on failed submit
Existing picker select path unchanged
No new dependencies

None.

Closes #98

## Summary - The property picker (PR #66) replaced the unified search+quick-add input but lost the Enter-to-create path for new properties - The controller already handled `client_name` param — this wires the UI back up - Type a name in the search box, hit Enter, property is created + queued in one action ## Changes - `app/views/work_queue_items/index.html.erb` — Add hidden `client_name` field and Stimulus targets to the picker form, add `keydown` action binding - `app/javascript/controllers/property_picker_controller.js` — Add `submit()` Enter handler that sends `client_name` instead of `property_id`, add form/field targets to replace `querySelector` calls - `spec/requests/work_queue_items_spec.rb` — Add request spec covering the quick-add turbo stream path (20 examples, 0 failures) ## Test Plan - [x] `docker compose run --rm -e RAILS_ENV=test web bundle exec rspec` — 20 examples, 0 failures - [x] Verified in Playwright on `https://landscaping-dev.tail5b443a.ts.net/today` — typed "New Test Client", pressed Enter, property created + queued + appeared in Recent with "Queued" badge - [x] Existing property picker select (click to queue) still works - [x] Existing add buttons in Recent and Last Thursday sections unaffected ## Review Checklist - [x] Quick-add creates property AND queues it in one action - [x] Input clears after successful submit - [x] Input restores on failed submit - [x] Existing picker select path unchanged - [x] No new dependencies ## Related Notes None. ## Related Closes #98

ldraney added 1 commit

Restore quick-add new property from Today tab

ci/woodpecker/push/woodpecker Pipeline was successful

Details

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

ee90582cd3

The property picker (PR #66) replaced the unified search+quick-add input
but lost the Enter-to-create path. The controller already handles
client_name param — this wires the UI back up.

- Add hidden client_name field and Stimulus targets to the picker form
- Add Enter keydown handler that submits client_name for new properties
- Add request spec covering the quick-add turbo stream path

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

ldraney merged commit da99af8487 into main

2026-06-04 18:12:32 +00:00

ldraney deleted branch fix-quick-add-from-today-tab

ldraney referenced this pull request from a commit

Restore quick-add new property from Today tab (#105)

Rows
Columns

Restore quick-add new property from Today tab #105