Skip to content

Centralize form submit handling#1802

Open
ThomasSarlin wants to merge 10 commits intomainfrom
fix-1727-rework
Open

Centralize form submit handling#1802
ThomasSarlin wants to merge 10 commits intomainfrom
fix-1727-rework

Conversation

@ThomasSarlin
Copy link
Copy Markdown
Contributor

WHY

Adds to #1727 — form inputs were cleared even when the server returned an error, forcing users to re-enter all values

HOW

  • Introduces a shared HTTP.submitForm() helper that centralizes the submit-then-reset pattern and only resets the form on success, it is also made async for the possibility of add chaining for when the submitForm method is completed.
  • Refactors all form submit handlers across the management UI to use the new helper
  • Adds Playwright specs covering both the success (form resets) and error (form preserves values) behaviors for exchanges and queues forms

Test plan

  • Run Playwright specs: make test-frontend (or npx playwright test spec/frontend/submit_form.spec.js)
  • Verify exchanges form: create an exchange successfully — form should reset; attempt to create one that fails (e.g. no permission) — form should keep your input
  • Verify queues form: same success/error check
  • Spot-check other forms (policies, shovels, federation upstreams, users, bindings, vhosts) to confirm they still submit and reset correctly on success

@claude
Copy link
Copy Markdown

claude bot commented Mar 10, 2026

PR Review

Bug: Chained .then() runs on error responses in user.js and queues.js

submitForm resolves (not rejects) on most HTTP errors. When request() gets a non-OK response, standardErrorHandler only throws for 404 and unrecognized errors. For common errors like 403 (which have a reason), it calls alertErrorHandler and returns, so request() resolves with an { is_error: true } object. Then submitForm's inner .then() does if (res?.is_error) return — which resolves the outer promise with undefined, meaning any chained .then() still executes.

static/js/user.js:96-101 — On a failed updateUser form submit (e.g. 403), the chained .then() still runs:

HTTP.submitForm(evt.target, 'PUT', url, {
    body,
    toast: 'User updated'
  }).then(function () {
    updateUser()       // <-- calls API to refresh user data on error
    pwd.disabled = false  // <-- resets password field state on error
    pwd.required = true
  })

Before the refactor, these side effects were guarded by the success check. Now updateUser() fires and password field state is mutated even when the request fails.

static/js/queues.js:173-177 — Same pattern, lower impact since setting the vhost value on a non-reset form is effectively a no-op, but still executes code that should only run on success:

  }).then(function () {
    evt.target.querySelector('select[name="vhost"]').value = decodeURIComponent(vhost)
  })

Fix: Either (a) have submitForm reject (throw) when is_error is true so chained .then() is skipped, or (b) pass a callback option to submitForm that runs inside the success guard. Option (a) is cleaner:

async function submitForm (form, method, url, options = {}) {
  const { body, table, toast } = options
  const res = await request(method, url, { body })
  if (res?.is_error) return Promise.reject(res)
  if (table) table.reload()
  if (toast) DOM.toast(toast)
  form.reset()
}

Note: this change would also affect the exchange.js and queue.js .catch() handlers — they'd now also catch is_error responses (not just thrown 404s), which is arguably more correct behavior.

@claude
Copy link
Copy Markdown

claude bot commented Mar 10, 2026

Code Review

No issues found.

The refactoring correctly centralizes form submission logic into HTTP.submitForm. Error handling is preserved: the is_error early-return prevents form reset on non-404 errors, and promise rejection propagates correctly for 404 .catch() handlers in exchange.js and queue.js. The callback-after-reset ordering in queues.js (vhost re-selection) and user.js (updateUser + pwd re-enable) maintains the same effective behavior as before.

@ThomasSarlin ThomasSarlin marked this pull request as ready for review March 10, 2026 07:27
@ThomasSarlin ThomasSarlin requested a review from a team as a code owner March 10, 2026 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants