fix: add isSaving to state, guard submit and catch errors

[tomrndom]

ref: https://app.clickup.com/t/9014802374/86batpx0h

Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com

Summary by CodeRabbit

• Bug Fixes
  • Prevented duplicate submissions when Save is clicked multiple times.
  • Kept the Save button disabled while changes are being submitted.
  • Re-enabled the Save button if saving fails, preventing the form from becoming stuck.

[coderabbitai]

📝 Walkthrough

Walkthrough

BadgeSettingsForm now prevents concurrent saves, manages asynchronous submission state, handles rejected submissions, and disables the Save button during saves. Tests cover duplicate clicks and button re-enablement after rejection.

Changes

Badge settings save flow

Layer / File(s)	Summary
Save state and submission flow src/components/forms/badge-settings-form.js	Adds isSaving, prevents concurrent submissions, handles success and rejection, resets state in finally, and disables the Save button while saving.
Save flow interaction tests src/components/forms/__tests__/badge-settings-form.test.js	Mocks integrations and tests duplicate-click prevention plus Save button re-enablement after rejection.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BadgeSettingsForm
  participant onSubmit
  participant Swal.fire
  BadgeSettingsForm->>BadgeSettingsForm: Set isSaving to true
  BadgeSettingsForm->>onSubmit: Submit updated entity fields
  onSubmit-->>BadgeSettingsForm: Resolve or reject
  BadgeSettingsForm->>Swal.fire: Show success alert on resolve
  BadgeSettingsForm->>BadgeSettingsForm: Reset isSaving in finally

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: saving-state guard and error handling for form submission.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/badge-settings-double-save

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/components/forms/badge-settings-form.js`:
- Around line 172-188: Update handleSubmit’s onSubmit flow to create a Promise
boundary with Promise.resolve().then(() => this.props.onSubmit(settingsToSave)),
preserving the existing success, catch, and finally handlers so synchronous
throws or non-Promise returns always reset isSaving.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bd39d0f8-2444-4382-9089-0b940b4d0b5f

📥 Commits

Reviewing files that changed from the base of the PR and between 662c1d2 and a473878.

📒 Files selected for processing (2)

• src/components/forms/__tests__/badge-settings-form.test.js
• src/components/forms/badge-settings-form.js

[smarcet]

@tomrndom .catch(() => {}) on this line sits after .then(), so it swallows errors thrown inside the success handler (the block above), not just rejections from onSubmit. If Swal.fire(success_message) throws for any reason (bad message shape, a sweetalert2 internal error), that error is silently absorbed here: isSaving still resets via .finally(), but the user sees neither a success nor an error message, and the bug never surfaces in the console — a real save success ends up looking identical to a swallowed exception.

Suggested fix — use the two-argument form of .then() so the reject handler only covers onSubmit's own rejection, letting a success-handler error propagate instead of being masked:

this.props
  .onSubmit(settingsToSave)
  .then(
    () => {
      Swal.fire({
        title: T.translate("general.done"),
        html: T.translate("badge_settings.badge_template_settings_updated"),
        type: "success"
      });
    },
    () => {} // only swallows onSubmit's own rejection
  )
  .finally(() => {
    this.setState({ isSaving: false });
  });

Regression test to add alongside it in badge-settings-form.test.js — proves the success-handler error is no longer swallowed (fails on the current .then().catch() chain, passes after the fix above):

import Swal from "sweetalert2";

it("should not swallow an error thrown by the success handler", async () => {
  const onSubmit = jest.fn(() => Promise.resolve());
  Swal.fire.mockImplementationOnce(() => {
    throw new Error("Swal render error");
  });

  const rejections = [];
  const onUnhandledRejection = (event) => rejections.push(event.reason);
  window.addEventListener("unhandledrejection", onUnhandledRejection);

  const { container } = renderForm(onSubmit);
  fireEvent.change(container.querySelector("#BADGE_TEMPLATE_WIDTH"), {
    target: { value: "100" }
  });
  fireEvent.click(screen.getByRole("button", { name: "general.save" }));

  await waitFor(() => expect(rejections).toHaveLength(1));
  window.removeEventListener("unhandledrejection", onUnhandledRejection);
});

[smarcet]

@tomrndom please review