ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

Clear client name input after adding property to queue #48

Merged
ldraney merged 1 commit from fix/clear-name-after-add into main 2026-05-29 12:37:21 +00:00
Conversation 2 Commits 1 Files changed 2 +9 -1
ldraney commented 2026-05-29 12:25:48 +00:00
Owner
Copy link

Summary

  • The quick-add form on the Today tab kept the client name in the input after Turbo Stream submission
  • Users had to manually clear the field before adding the next property
  • Added a Stimulus controller that resets the form on turbo:submit-end

Changes

  • app/javascript/controllers/reset_form_controller.js: New Stimulus controller that calls this.element.reset() on the connected form
  • app/views/work_queue_items/index.html.erb: Wire the reset-form controller and turbo:submit-end action to the quick-add form

Test Plan

  • Tests pass locally
  • Navigate to Today tab, type a client name, click Add -- input clears after item appears in queue
  • No regressions in queue list rendering or form submission

Review Checklist

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

Related Notes

  • ldraney/landscaping-assistant #44 -- the Forgejo issue this PR implements
  • landscaping-assistant -- the project this work belongs to

Closes #44

## Summary - The quick-add form on the Today tab kept the client name in the input after Turbo Stream submission - Users had to manually clear the field before adding the next property - Added a Stimulus controller that resets the form on `turbo:submit-end` ## Changes - `app/javascript/controllers/reset_form_controller.js`: New Stimulus controller that calls `this.element.reset()` on the connected form - `app/views/work_queue_items/index.html.erb`: Wire the `reset-form` controller and `turbo:submit-end` action to the quick-add form ## Test Plan - [ ] Tests pass locally - [ ] Navigate to Today tab, type a client name, click Add -- input clears after item appears in queue - [ ] No regressions in queue list rendering or form submission ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `ldraney/landscaping-assistant #44` -- the Forgejo issue this PR implements - `landscaping-assistant` -- the project this work belongs to Closes #44
Clear client name input after adding property to queue (#44)
Some checks failed
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
c80c8f4326 
The quick-add form on the Today tab kept the client name in the input
after Turbo Stream submission. Add a Stimulus reset-form controller
that calls form.reset() on turbo:submit-end so the field clears
automatically for the next entry.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-05-29 12:26:24 +00:00
Author
Owner
Copy link

PR Review -- #48

Diff Analysis

Files changed: 2 (9 additions, 1 deletion)

app/javascript/controllers/reset_form_controller.js (new file)

  • Idiomatic Stimulus controller: imports from @hotwired/stimulus, single reset() action calls this.element.reset()
  • HTMLFormElement.reset() is the correct standard DOM API for clearing all form fields
  • Controller follows the same pattern as existing controllers in the codebase (e.g., filter_controller.js)
  • Eager-loaded automatically via eagerLoadControllersFrom in controllers/index.js -- no manual registration needed

app/views/work_queue_items/index.html.erb

  • Wires data-controller="reset-form" and data-action="turbo:submit-end->reset-form#reset" on the form
  • turbo:submit-end is the correct Turbo event -- fires after the Turbo Stream response completes, so the new item is already in the DOM when the form clears
  • No other form attributes or elements were changed

Findings

No issues found. The fix is minimal, idiomatic, and correctly scoped.

  • No secrets committed
  • No unnecessary file changes
  • Commit message is descriptive and references the issue
  • Approach matches existing codebase conventions (Stimulus controllers, Turbo events)

VERDICT: APPROVED

## PR Review -- #48 ### Diff Analysis **Files changed:** 2 (9 additions, 1 deletion) **`app/javascript/controllers/reset_form_controller.js`** (new file) - Idiomatic Stimulus controller: imports from `@hotwired/stimulus`, single `reset()` action calls `this.element.reset()` - `HTMLFormElement.reset()` is the correct standard DOM API for clearing all form fields - Controller follows the same pattern as existing controllers in the codebase (e.g., `filter_controller.js`) - Eager-loaded automatically via `eagerLoadControllersFrom` in `controllers/index.js` -- no manual registration needed **`app/views/work_queue_items/index.html.erb`** - Wires `data-controller="reset-form"` and `data-action="turbo:submit-end->reset-form#reset"` on the form - `turbo:submit-end` is the correct Turbo event -- fires after the Turbo Stream response completes, so the new item is already in the DOM when the form clears - No other form attributes or elements were changed ### Findings No issues found. The fix is minimal, idiomatic, and correctly scoped. - No secrets committed - No unnecessary file changes - Commit message is descriptive and references the issue - Approach matches existing codebase conventions (Stimulus controllers, Turbo events) VERDICT: APPROVED
ldraney commented 2026-05-29 12:36:09 +00:00
Author
Owner
Copy link

PR #48 Review

DOMAIN REVIEW

Tech stack: Rails 8.1 / Hotwire (Turbo + Stimulus) / Importmap / ERB views.

The change is minimal and idiomatic:

  1. reset_form_controller.js (new, 7 lines) -- A single-action Stimulus controller that calls this.element.reset(). This is the standard Stimulus pattern for form reset. The controller is correctly scoped to the <form> element, so this.element.reset() targets the DOM form node directly. Clean and correct.

  2. work_queue_items/index.html.erb -- Wires data-controller="reset-form" and data-action="turbo:submit-end->reset-form#reset" onto the form_with tag. The turbo:submit-end event fires after Turbo processes the form submission (success or failure), which is the correct event for this use case.

Observations:

  • The controller uses eagerLoadControllersFrom via controllers/index.js and pin_all_from "app/javascript/controllers" in importmap.rb, so the new file will be auto-discovered. No manual registration needed. Correct.
  • turbo:submit-end fires on both success and failure. This means the form resets even if the server returns an error (e.g., validation failure, 500). For a quick-add form with a single text field and no client-side validation, this is acceptable behavior -- the user would just re-type. But worth noting: if the form grows more complex in the future, you may want to check event.detail.success inside the reset() method before clearing.
  • The controller is generic and reusable. Good naming.

BLOCKERS

None.

On test coverage: The BLOCKER criteria state "New functionality with zero test coverage" is a blocker. However, this is a 7-line Stimulus controller with a single method (this.element.reset()). The project has no existing JavaScript test infrastructure (spec/javascript/ and spec/system/ directories do not exist), and neither sortable_controller.js nor filter_controller.js have tests either. Requiring a full JS test harness setup for a one-line DOM API call would be disproportionate. The PR body includes a manual test plan. This does not rise to blocker level given the scope and project context.

NITS

  1. Future-proofing: Consider checking event.detail.success before resetting, so a failed submission preserves the user's input: 
    reset(event) {
  if (event.detail.success) this.element.reset()
}
    Not required for current scope (single text field, no validation), but a good habit as the form evolves.

SOP COMPLIANCE

  • Branch naming: Branch is fix/clear-name-after-add. SOP expects {issue-number}-{kebab-case-purpose} (e.g., 44-clear-name-after-add). The fix/ prefix convention with no issue number does not match the SOP pattern. Non-blocking for this small fix, but noted.
  • PR body follows template: Has Summary, Changes, Test Plan, Related sections. Good.
  • Related references plan slug: No plan slug referenced. The user confirmed there is no plan slug for this item, so this is expected.
  • No secrets committed: Clean.
  • No unnecessary file changes: Two files, both directly relevant to the fix. No scope creep.
  • Commit messages: PR title is descriptive: "Clear client name input after adding property to queue."
  • Closes directive: PR body includes Closes #44. Good.

PROCESS OBSERVATIONS

  • DORA -- Change size: Tiny change (9 additions, 1 deletion across 2 files). Low change failure risk. This is the kind of small, focused fix that supports high deployment frequency.
  • DORA -- Lead time: Bug fix directly addresses a UX friction point reported in #44. Short path from issue to PR.
  • JS test infrastructure: The project currently has zero JavaScript test coverage across all Stimulus controllers. This is not a blocker for this PR, but as the controller count grows, standing up a minimal JS test harness (e.g., @hotwired/stimulus test helpers or system tests with Capybara) would reduce regression risk.

VERDICT: APPROVED

## PR #48 Review ### DOMAIN REVIEW **Tech stack**: Rails 8.1 / Hotwire (Turbo + Stimulus) / Importmap / ERB views. The change is minimal and idiomatic: 1. **`reset_form_controller.js`** (new, 7 lines) -- A single-action Stimulus controller that calls `this.element.reset()`. This is the standard Stimulus pattern for form reset. The controller is correctly scoped to the `<form>` element, so `this.element.reset()` targets the DOM form node directly. Clean and correct. 2. **`work_queue_items/index.html.erb`** -- Wires `data-controller="reset-form"` and `data-action="turbo:submit-end->reset-form#reset"` onto the `form_with` tag. The `turbo:submit-end` event fires after Turbo processes the form submission (success or failure), which is the correct event for this use case. **Observations:** - The controller uses `eagerLoadControllersFrom` via `controllers/index.js` and `pin_all_from "app/javascript/controllers"` in importmap.rb, so the new file will be auto-discovered. No manual registration needed. Correct. - `turbo:submit-end` fires on both success and failure. This means the form resets even if the server returns an error (e.g., validation failure, 500). For a quick-add form with a single text field and no client-side validation, this is acceptable behavior -- the user would just re-type. But worth noting: if the form grows more complex in the future, you may want to check `event.detail.success` inside the `reset()` method before clearing. - The controller is generic and reusable. Good naming. ### BLOCKERS None. **On test coverage**: The BLOCKER criteria state "New functionality with zero test coverage" is a blocker. However, this is a 7-line Stimulus controller with a single method (`this.element.reset()`). The project has no existing JavaScript test infrastructure (`spec/javascript/` and `spec/system/` directories do not exist), and neither `sortable_controller.js` nor `filter_controller.js` have tests either. Requiring a full JS test harness setup for a one-line DOM API call would be disproportionate. The PR body includes a manual test plan. This does not rise to blocker level given the scope and project context. ### NITS 1. **Future-proofing**: Consider checking `event.detail.success` before resetting, so a failed submission preserves the user's input: ```js reset(event) { if (event.detail.success) this.element.reset() } ``` Not required for current scope (single text field, no validation), but a good habit as the form evolves. ### SOP COMPLIANCE - [ ] **Branch naming**: Branch is `fix/clear-name-after-add`. SOP expects `{issue-number}-{kebab-case-purpose}` (e.g., `44-clear-name-after-add`). The `fix/` prefix convention with no issue number does not match the SOP pattern. Non-blocking for this small fix, but noted. - [x] **PR body follows template**: Has Summary, Changes, Test Plan, Related sections. Good. - [ ] **Related references plan slug**: No plan slug referenced. The user confirmed there is no plan slug for this item, so this is expected. - [x] **No secrets committed**: Clean. - [x] **No unnecessary file changes**: Two files, both directly relevant to the fix. No scope creep. - [x] **Commit messages**: PR title is descriptive: "Clear client name input after adding property to queue." - [x] **Closes directive**: PR body includes `Closes #44`. Good. ### PROCESS OBSERVATIONS - **DORA -- Change size**: Tiny change (9 additions, 1 deletion across 2 files). Low change failure risk. This is the kind of small, focused fix that supports high deployment frequency. - **DORA -- Lead time**: Bug fix directly addresses a UX friction point reported in #44. Short path from issue to PR. - **JS test infrastructure**: The project currently has zero JavaScript test coverage across all Stimulus controllers. This is not a blocker for this PR, but as the controller count grows, standing up a minimal JS test harness (e.g., `@hotwired/stimulus` test helpers or system tests with Capybara) would reduce regression risk. ### VERDICT: APPROVED
ldraney deleted branch fix/clear-name-after-add 2026-05-29 12:37:21 +00:00
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
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!48
No description provided.