Skip to content

fix(settings): report counters load failures - #1505

Open
khaira777 wants to merge 2 commits into
mainfrom
fix/counters-load-failure-toast
Open

khaira777 wants to merge 2 commits into
mainfrom
fix/counters-load-failure-toast

Conversation

@khaira777

Copy link
Copy Markdown
Contributor

Related work

Issue / Discussion: None (audit finding F-15)

Summary

CountersSettingsTab wrapped the /settings/multi_counter_enabled and /counters requests in per-request .catch() fallbacks. A 429, 500 or network failure therefore silently showed multi-counter as disabled with an empty list, and the existing counterLoadFailed toast never ran. An owner could think the counters were gone and recreate them.

The fallbacks are removed in both loadData and the mount effect, so failures reach the existing catch. The fallbacks were not needed for a missing key, because GET /settings/multi_counter_enabled already returns the 'false' default from OPTIONAL_SETTING_DEFAULTS.

Scope

No new failed-load or retry UI. The tab keeps its default state and now shows the error toast.

Verification

Commands run:

  • npm run lint, npm run build:frontend
  • Playwright on isolated ports: npx playwright test e2e/settings-request-budget.spec.ts --project=chromium
  • oxfmt --check, git diff --check

Results: all passed. The new test "Counters tab reports a failed counters load instead of showing an empty list" fails without the fix (checked against a frontend build with the fix removed). It sits at the end of the spec file so it doesn't conflict with the sibling Save All phone-validation PR.

Compatibility & data safety

  • Database/data impact: None
  • Migration: None
  • User-visible behavior: A failed counters load shows "Failed to load counters."
  • Offline/network impact: None

Cross-cutting surfaces

Surface Status
UI affected
API not affected
Persistence not affected
Permissions not affected

Developer documentation

  • No developer documentation change needed
  • Existing developer documentation updated
  • New page justified
  • Not applicable (docs-only or code-only change)

Contributor checklist

  • This is a small isolated fix/doc update OR follows an approved issue/direction.
  • Unrelated cleanups, refactors, and dependency changes were kept out of this PR.
  • Tests were added or updated for any behavior changes.
  • Verification commands were run and documented above.
  • Customer data and upgrade safety were preserved (if touching database/storage). (N/A)
  • Documentation or translations were updated where relevant.
  • I understand and can explain all submitted changes.

Per-request catch fallbacks turned a failed /counters or multi_counter_enabled load into disabled mode with an empty list, so the counterLoadFailed toast never ran. Let failures reach it; the settings route already returns the default for a missing key.
@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Report counters settings load failures

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Let failed counters or multi-counter setting requests reach the existing error toast instead of
 appearing successful.
• Add a browser test confirming a failed counters request displays the load-failure message.
Diagram

graph TD
  Tab["Counters tab"] --> Fetch["Parallel requests"] --> Setting["Settings API"] --> Result{"Load succeeded?"}
  Fetch --> Counters["Counters API"] --> Result
  Result -->|Yes| View["Counter state"]
  Result -->|No| Toast["Failure toast"]
Loading
High-Level Assessment

Use the existing catch handlers. The API already supplies the default for a missing setting, so per-request fallbacks are unnecessary; a new error or retry UI would expand this focused fix.

Files changed (2) +20 / -12

Bug fix (1) +4 / -12
CountersSettingsTab.tsxStop masking counter-load request failures +4/-12

Stop masking counter-load request failures

• Removes per-request fallback data from both the initial load and subsequent reload. Rejected requests now reach the existing error toast instead of being treated as disabled multi-counter mode or an empty list.

frontend/src/components/settings/CountersSettingsTab.tsx

Tests (1) +16 / -0
settings-request-budget.spec.tsCover a failed counters load in the browser +16/-0

Cover a failed counters load in the browser

• Adds a Playwright test that returns HTTP 500 for counters requests and verifies the Counters tab displays its load-failure message.

frontend/e2e/settings-request-budget.spec.ts

@qodo-code-review

qodo-code-review Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Failed load shows an empty counters list ✓ Resolved

Resolving commit: 60c794b

Resolution reason
Switched both load paths to Promise.allSettled, applying fulfilled results independently; added
loadError state that disables the toggle and Add button and shows an error with a retry action
instead of the empty list.
Description
The catch branch of loadData and the mount effect's .catch only fire a transient toast, while
Promise.all now rejects if either /settings/multi_counter_enabled or /counters fails, so the
successful response is discarded and neither state setter runs. Because counters stays [] and
enabled stays false, the Add and toggle controls remain active on that default state, and a 429
or 500 on the settings read now also hides a counters list that loaded successfully.

Grey Divider

Context sources
✅ Compliance rules (platform): 5 rules
✅ Cross-repo context — repo relationships
Review mode: Auto: 🚀 Fast: Small, localized error-handling change in one settings component; no high-risk areas.

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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.

1 participant