Skip to content

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

Open
tomrndom wants to merge 5 commits into
masterfrom
fix/badge-settings-double-save
Open

fix: add isSaving to state, guard submit and catch errors#1010
tomrndom wants to merge 5 commits into
masterfrom
fix/badge-settings-double-save

Conversation

@tomrndom

@tomrndom tomrndom commented Jul 15, 2026

Copy link
Copy Markdown

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 when saving fails, preventing the form from becoming stuck.
    • Improved submission and success notification error handling.
    • Ensured all badge-setting updates finish before reporting failures.
    • Preserved reliable form behavior when users navigate away during submission.

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

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1a5deb3a-bd8b-47b8-abb8-dd2d432bcd7a

📥 Commits

Reviewing files that changed from the base of the PR and between dc08235 and af7f977.

📒 Files selected for processing (1)
  • src/actions/__tests__/badge-actions.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/actions/tests/badge-actions.test.js

📝 Walkthrough

Walkthrough

BadgeSettingsForm now prevents concurrent saves, manages submission state, handles rejected submissions, and disables Save while saving. saveBadgeSettings waits for all operations to settle before reporting rejection. Tests cover these behaviors.

Changes

Badge settings save flow

Layer / File(s) Summary
Save action settlement
src/actions/badge-actions.js, src/actions/__tests__/badge-actions.test.js
saveBadgeSettings waits for all limited operations to settle, throws the first rejection after completion, and returns fulfilled values in input order. Tests cover delayed rejection and successful completion.
Save state and submission flow
src/components/forms/badge-settings-form.js
Adds saving and mount-state tracking, prevents concurrent submissions, handles success and rejection, and disables Save while saving.
Save flow interaction tests
src/components/forms/__tests__/badge-settings-form.test.js
Mocks integrations and tests duplicate-click prevention, rejection recovery, distinct callbacks, success-handler errors, and successful-save feedback.

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

Sequence Diagram(s)

sequenceDiagram
  participant BadgeSettingsForm
  participant saveBadgeSettings
  participant Swal.fire
  BadgeSettingsForm->>BadgeSettingsForm: Set isSaving to true
  BadgeSettingsForm->>saveBadgeSettings: Submit badge settings
  saveBadgeSettings-->>BadgeSettingsForm: Settle operations and return or reject
  BadgeSettingsForm->>Swal.fire: Show success message on resolve
  BadgeSettingsForm->>BadgeSettingsForm: Reset isSaving when mounted
Loading

Possibly related PRs

  • fntechgit/summit-admin#918: Related save-submission state handling prevents duplicate submissions and re-enables controls after promise completion or rejection.
🚥 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 clearly summarizes the main changes: adding isSaving state, guarding submissions, and catching errors.
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 coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/components/forms/badge-settings-form.js
@tomrndom
tomrndom requested a review from smarcet July 15, 2026 13:06
Comment thread src/components/forms/badge-settings-form.js Outdated

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom please review

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

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom please review

Comment thread src/components/forms/badge-settings-form.js
Comment thread src/components/forms/badge-settings-form.js Outdated
Comment thread src/components/forms/__tests__/badge-settings-form.test.js
…nted, add test for success path

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
…tled

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/actions/__tests__/badge-actions.test.js`:
- Around line 72-83: Update the saveBadgeSettings test around
saveMarketingSetting to mock distinct fulfilled results for settings a and b,
then assert the dispatch promise resolves to an array containing those results
in a-then-b input order instead of only checking that the value is defined.
🪄 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: 94afd55a-d78a-43ce-b0a9-7a540b4f59f8

📥 Commits

Reviewing files that changed from the base of the PR and between 727eda1 and dc08235.

📒 Files selected for processing (1)
  • src/actions/__tests__/badge-actions.test.js

Comment thread src/actions/__tests__/badge-actions.test.js Outdated
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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