Fix quick-add form reset on duplicate queue submission

ldraney commented

2026-06-04 04:51:05 +00:00

Owner

Closes #50

Summary

The create action's failure path used redirect_to, causing Turbo to reload the entire page — resetting the property picker input and losing state
Changed failure path to respond with turbo_stream (status 422) so the Stimulus controller's existing turbo:submit-end handler can roll back the optimistic UI in-place
Added inline flash toast ("Already in queue.") via a shared partial with auto-dismiss after 3s
Added #flash-messages container to layout and fixed positioning CSS for overlay toasts

Changes

app/controllers/work_queue_items_controller.rb — failure branch now responds with turbo_stream + flash partial instead of redirect
app/views/shared/_flash.html.erb — new shared flash partial
app/javascript/controllers/auto_dismiss_controller.js — removes flash after 3s
app/views/layouts/application.html.erb — added #flash-messages container
app/assets/stylesheets/application.css — fixed-position flash container
spec/requests/work_queue_items_spec.rb — 2 new specs (turbo_stream error response, no duplicate creation)

Test Plan

88 specs pass (0 failures), including 2 new specs for the failure path
RuboCop clean on changed Ruby files
Manual: add a property to queue, try adding same property again — should see "Already in queue." toast, picker stays open, no page reload

Review Checklist

No Turbo Drive redirect on failure — stays on page
Stimulus turbo:submit-end handler rolls back optimistic UI on 422
Flash auto-dismisses after 3 seconds
HTML fallback still works (redirect for non-Turbo clients)
No new gems or dependencies

Parent issue: #50 (Quick-add form resets on failed submissions)
The property_picker_controller.js already had turbo:submit-end failure handling (lines 55-61) but it couldn't fire properly because the redirect replaced the DOM

Closes #50 ## Summary - The create action's failure path used `redirect_to`, causing Turbo to reload the entire page — resetting the property picker input and losing state - Changed failure path to respond with `turbo_stream` (status 422) so the Stimulus controller's existing `turbo:submit-end` handler can roll back the optimistic UI in-place - Added inline flash toast ("Already in queue.") via a shared partial with auto-dismiss after 3s - Added `#flash-messages` container to layout and fixed positioning CSS for overlay toasts ## Changes - `app/controllers/work_queue_items_controller.rb` — failure branch now responds with turbo_stream + flash partial instead of redirect - `app/views/shared/_flash.html.erb` — new shared flash partial - `app/javascript/controllers/auto_dismiss_controller.js` — removes flash after 3s - `app/views/layouts/application.html.erb` — added `#flash-messages` container - `app/assets/stylesheets/application.css` — fixed-position flash container - `spec/requests/work_queue_items_spec.rb` — 2 new specs (turbo_stream error response, no duplicate creation) ## Test Plan - [x] 88 specs pass (0 failures), including 2 new specs for the failure path - [x] RuboCop clean on changed Ruby files - [ ] Manual: add a property to queue, try adding same property again — should see "Already in queue." toast, picker stays open, no page reload ## Review Checklist - [x] No Turbo Drive redirect on failure — stays on page - [x] Stimulus `turbo:submit-end` handler rolls back optimistic UI on 422 - [x] Flash auto-dismisses after 3 seconds - [x] HTML fallback still works (redirect for non-Turbo clients) - [x] No new gems or dependencies ## Related Notes - Parent issue: #50 (Quick-add form resets on failed submissions) - The property_picker_controller.js already had `turbo:submit-end` failure handling (lines 55-61) but it couldn't fire properly because the redirect replaced the DOM

ldraney added 1 commit

2026-06-04 04:51:05 +00:00

Fix quick-add form reset on duplicate queue submission (#50 )

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

Details

CI / lint (pull_request) Has been cancelled

Details

4c6e4f4a1a

The create action's failure path used redirect_to, which caused Turbo to
reload the page — resetting the property picker and losing input state.
Now responds with turbo_stream (422) so the Stimulus controller can roll
back the optimistic UI in-place and show an inline error toast.

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

ldraney commented

2026-06-04 04:52:42 +00:00

Author

Owner

PR #80 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8 / Hotwire (Turbo Streams + Stimulus) / importmap / RSpec

Controller (work_queue_items_controller.rb)

The fix correctly replaces the redirect_to failure path with a respond_to block that renders turbo_stream with status :unprocessable_entity (422). This is the canonical Hotwire pattern for form validation failures -- the 422 status lets the Stimulus turbo:submit-end handler in property_picker_controller.js (line 56) detect !e.detail.success and roll back the optimistic UI.
The HTML fallback (format.html { redirect_to ... }) is preserved for non-Turbo clients. Good.
The error message sourcing (@item.errors.full_messages.first || "Already in queue.") is correct. The model validates property_id uniqueness scoped to work_date, so the validation message will populate properly. The fallback string handles edge cases.

Flash partial (shared/_flash.html.erb)

Uses <%= message %> which auto-escapes HTML in ERB. No XSS risk.
The data-controller="auto-dismiss" wiring is correct -- the controller file follows the Stimulus naming convention (auto_dismiss_controller.js maps to auto-dismiss).

Stimulus controller (auto_dismiss_controller.js)

Auto-registered via eagerLoadControllersFrom + pin_all_from in the importmap. No manual registration needed.
setTimeout(() => this.element.remove(), 3000) is clean. The element is removed from the DOM entirely, which is appropriate for a toast.

Layout (application.html.erb)

<div id="flash-messages"></div> placed correctly at the top of <body>, before <main>. The Turbo Stream append action targets this container by ID.

CSS

Fixed positioning with z-index: 1000 is appropriate for overlay toasts. Uses theme tokens (--spacing-xs, --spacing-md, --radius, --color-danger, --color-danger-light) -- no magic numbers. Good.

Tests (spec/requests/work_queue_items_spec.rb)

Two new specs cover both the turbo_stream error response (status 422, correct media type, flash content) and the duplicate prevention (count unchanged). These test the exact behavior described in the bug.
The duplicate test uses the default HTML accept header (no turbo_stream header), which exercises the format.html fallback path.

BLOCKERS

None.

NITS

Timer cleanup on disconnect (auto_dismiss_controller.js): If the element is removed from the DOM before the 3s timeout fires (e.g., user navigates away via Turbo Drive), the setTimeout callback will call remove() on a detached element. This is harmless (no error thrown), but for hygiene, consider storing the timer ID and clearing it in a disconnect() callback:
```
connect() {
  this.timer = setTimeout(() => this.element.remove(), 3000)
}
disconnect() {
  clearTimeout(this.timer)
}
```
Non-blocking -- the current code works fine.
Flash partial is hardcoded to flash-alert: The partial always renders with class flash-alert. If this partial is reused for success messages in the future, it would need a type local. Fine for now since the only caller is the error path, but worth noting if you plan to reuse shared/flash elsewhere.
No ARIA role on flash toast: The flash container could benefit from role="alert" or role="status" for screen reader announcements. e.g., <div class="flash flash-alert" role="alert" ...>. Non-blocking, but would improve accessibility.

SOP COMPLIANCE

Branch named after issue: 50-fix-quick-add-reset follows {issue-number}-{kebab-case-purpose}
PR body follows template: Summary, Changes, Test Plan, Related sections all present
Related references plan slug: Related section references issue #50 but no plan slug. Acceptable for a bug fix without a parent plan.
No secrets committed
No scope creep: all 6 changed files directly support the fix
Commit messages are descriptive (PR title is clear)

PROCESS OBSERVATIONS

Change failure risk: Low. The fix is narrowly scoped to the controller failure path. The success path is untouched. Both turbo_stream and HTML fallback are covered.
Deployment frequency: Neutral. Small, self-contained fix. No new dependencies or migrations.
Test coverage is solid. Two new request specs cover the exact regression described in #50. The existing Stimulus controller already had the client-side failure handling (lines 55-61) -- this fix simply ensures the server responds in a way that triggers it.

VERDICT: APPROVED

## PR #80 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8 / Hotwire (Turbo Streams + Stimulus) / importmap / RSpec **Controller (`work_queue_items_controller.rb`)** - The fix correctly replaces the `redirect_to` failure path with a `respond_to` block that renders `turbo_stream` with status `:unprocessable_entity` (422). This is the canonical Hotwire pattern for form validation failures -- the 422 status lets the Stimulus `turbo:submit-end` handler in `property_picker_controller.js` (line 56) detect `!e.detail.success` and roll back the optimistic UI. - The HTML fallback (`format.html { redirect_to ... }`) is preserved for non-Turbo clients. Good. - The error message sourcing (`@item.errors.full_messages.first || "Already in queue."`) is correct. The model validates `property_id` uniqueness scoped to `work_date`, so the validation message will populate properly. The fallback string handles edge cases. **Flash partial (`shared/_flash.html.erb`)** - Uses `<%= message %>` which auto-escapes HTML in ERB. No XSS risk. - The `data-controller="auto-dismiss"` wiring is correct -- the controller file follows the Stimulus naming convention (`auto_dismiss_controller.js` maps to `auto-dismiss`). **Stimulus controller (`auto_dismiss_controller.js`)** - Auto-registered via `eagerLoadControllersFrom` + `pin_all_from` in the importmap. No manual registration needed. - `setTimeout(() => this.element.remove(), 3000)` is clean. The element is removed from the DOM entirely, which is appropriate for a toast. **Layout (`application.html.erb`)** - `<div id="flash-messages"></div>` placed correctly at the top of `<body>`, before `<main>`. The Turbo Stream `append` action targets this container by ID. **CSS** - Fixed positioning with `z-index: 1000` is appropriate for overlay toasts. Uses theme tokens (`--spacing-xs`, `--spacing-md`, `--radius`, `--color-danger`, `--color-danger-light`) -- no magic numbers. Good. **Tests (`spec/requests/work_queue_items_spec.rb`)** - Two new specs cover both the turbo_stream error response (status 422, correct media type, flash content) and the duplicate prevention (count unchanged). These test the exact behavior described in the bug. - The duplicate test uses the default HTML accept header (no turbo_stream header), which exercises the `format.html` fallback path. ### BLOCKERS None. ### NITS 1. **Timer cleanup on disconnect** (`auto_dismiss_controller.js`): If the element is removed from the DOM before the 3s timeout fires (e.g., user navigates away via Turbo Drive), the `setTimeout` callback will call `remove()` on a detached element. This is harmless (no error thrown), but for hygiene, consider storing the timer ID and clearing it in a `disconnect()` callback: ```js connect() { this.timer = setTimeout(() => this.element.remove(), 3000) } disconnect() { clearTimeout(this.timer) } ``` Non-blocking -- the current code works fine. 2. **Flash partial is hardcoded to `flash-alert`**: The partial always renders with class `flash-alert`. If this partial is reused for success messages in the future, it would need a `type` local. Fine for now since the only caller is the error path, but worth noting if you plan to reuse `shared/flash` elsewhere. 3. **No ARIA role on flash toast**: The flash container could benefit from `role="alert"` or `role="status"` for screen reader announcements. e.g., `<div class="flash flash-alert" role="alert" ...>`. Non-blocking, but would improve accessibility. ### SOP COMPLIANCE - [x] Branch named after issue: `50-fix-quick-add-reset` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [ ] Related references plan slug: Related section references issue #50 but no plan slug. Acceptable for a bug fix without a parent plan. - [x] No secrets committed - [x] No scope creep: all 6 changed files directly support the fix - [x] Commit messages are descriptive (PR title is clear) ### PROCESS OBSERVATIONS - **Change failure risk: Low.** The fix is narrowly scoped to the controller failure path. The success path is untouched. Both turbo_stream and HTML fallback are covered. - **Deployment frequency: Neutral.** Small, self-contained fix. No new dependencies or migrations. - **Test coverage is solid.** Two new request specs cover the exact regression described in #50. The existing Stimulus controller already had the client-side failure handling (lines 55-61) -- this fix simply ensures the server responds in a way that triggers it. ### VERDICT: APPROVED