fix(web): prevent duplicate system setting notifications#6076
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/default/src/features/system-settings/hooks/update-option-notification.ts (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
import { t } from 'i18next'per coding guidelines.The coding guidelines recommend
import { t } from 'i18next'in non-React code. Using the named import is more concise and aligns with the project convention.As per coding guidelines: "In non-React code (utility functions, constants, class methods), use
import { t } from 'i18next'only when the text does not need reactive language switching."♻️ Optional import refactor
-import i18next from 'i18next' +import { t } from 'i18next'Then replace
i18next.t(...)calls witht(...):- notification?.success || i18next.t('Setting updated successfully'), + notification?.success || t('Setting updated successfully'),- i18next.t('Failed to update setting'), + t('Failed to update setting'),🤖 Prompt for 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. In `@web/default/src/features/system-settings/hooks/update-option-notification.ts` around lines 19 - 20, Replace the default i18next import with the named t import in the update-option notification module, then update every i18next.t(...) call to use t(...), preserving the existing translation keys and behavior.Source: Coding guidelines
web/default/src/features/system-settings/api.test.ts (1)
1-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Vitest instead of
node:testper coding guidelines.The coding guidelines for
web/default/**/*.test.tsrequire Vitest for unit tests. This file usesnode:testandnode:assert/strict, and monkey-patchesapi.putmanually. Migrating to Vitest withvi.spyOnwould align with the guideline, eliminate the manual restore logic, and match the style used inupdate-option-notification.test.ts.As per coding guidelines: "Write unit tests for utility functions and pure logic with Vitest."
♻️ Proposed migration to Vitest
-import assert from 'node:assert/strict' -import { test } from 'node:test' - -import { api } from '`@/lib/api`' - -import { updateSystemOption } from './api' - -test('system option updates use one notification owner', async () => { - const originalPut = api.put - let requestConfig: Parameters<typeof api.put>[2] - - try { - api.put = (async (_url, _request, config) => { - requestConfig = config - return { - data: { success: true, message: '' }, - } - }) as typeof api.put - - await updateSystemOption({ key: 'SystemName', value: 'Example' }) - - assert.equal(requestConfig?.skipBusinessError, true) - assert.equal(requestConfig?.skipErrorHandler, true) - - api.put = (async () => ({ - data: { success: false, message: 'Update rejected' }, - })) as typeof api.put - - await assert.rejects( - updateSystemOption({ key: 'SystemName', value: 'Example' }), - /Update rejected/ - ) - } finally { - api.put = originalPut - } -}) +import { expect, test, vi } from 'vitest' + +import { api } from '`@/lib/api`' + +import { updateSystemOption } from './api' + +test('updateSystemOption bypasses shared error handlers and throws on business failure', async () => { + const putSpy = vi.spyOn(api, 'put') + + putSpy.mockResolvedValue({ data: { success: true, message: '' } } as never) + await updateSystemOption({ key: 'SystemName', value: 'Example' }) + + expect(putSpy).toHaveBeenCalledWith( + '/api/option/', + { key: 'SystemName', value: 'Example' }, + expect.objectContaining({ + skipBusinessError: true, + skipErrorHandler: true, + }) + ) + + putSpy.mockResolvedValue({ + data: { success: false, message: 'Update rejected' }, + } as never) + + await expect( + updateSystemOption({ key: 'SystemName', value: 'Example' }) + ).rejects.toThrow('Update rejected') +})🤖 Prompt for 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. In `@web/default/src/features/system-settings/api.test.ts` around lines 1 - 54, Replace the node:test and node:assert/strict imports in the system option update test with Vitest APIs, using vi.spyOn(api, 'put') to mock requests and restore the spy through Vitest cleanup rather than manual reassignment and try/finally. Preserve the existing success and rejection assertions, following the style of update-option-notification.test.ts.Source: Coding guidelines
web/default/src/features/system-settings/hooks/update-option-notification.test.ts (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Vitest instead of
node:testfor unit tests.The coding guidelines require
*.test.tsfiles underweb/default/to use Vitest. This file importsdescribe/testfromnode:testandassertfromnode:assert/strictinstead. Additionally,toast.getHistory()appears to be an internal sonner API — with Vitest you could mock thesonnermodule viavi.mock('sonner')and assert ontoast.success/toast.errorcall arguments, which would be more robust than relying on sonner's internal history.As per coding guidelines: "
web/default/**/*.test.ts: Write unit tests for utility functions and pure logic with Vitest."♻️ Migrate to Vitest with mocked sonner
-import assert from 'node:assert/strict' -import { describe, test } from 'node:test' - +import { beforeEach, describe, expect, it, vi } from 'vitest' + import { toast } from 'sonner' + +vi.mock('sonner', () => ({ + toast: { + success: vi.fn(), + error: vi.fn(), + }, +})) + import { showUpdateOptionError, showUpdateOptionSuccess, } from './update-option-notification' + +beforeEach(() => { + vi.clearAllMocks() +})Then replace assertions, e.g.:
- assert.equal(matchingToasts.length, 1) - assert.ok(matchingToast && 'title' in matchingToast) - assert.equal(matchingToast.title, 'Settings saved') + expect(toast.success).toHaveBeenCalledTimes(1) + expect(toast.success).toHaveBeenLastCalledWith('Settings saved', { + id: 'update-option-batch-success-test', + })🤖 Prompt for 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. In `@web/default/src/features/system-settings/hooks/update-option-notification.test.ts` around lines 19 - 20, Replace the node:test and node:assert imports in the update-option notification tests with Vitest APIs, using describe, test, expect, beforeEach, and vi as needed. Mock the sonner module with vi.mock('sonner'), reset mocks between tests, and assert toast.success or toast.error call arguments instead of relying on toast.getHistory().Source: Coding guidelines
🤖 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 `@web/default/src/features/system-settings/content/api-info-section.tsx`:
- Around line 247-249: Add a success value to the notification object used by
handleSaveAll in the API info save flow, using a specific translated success
message consistent with the announcements and FAQ sections, or set success:
false if no success toast is desired.
---
Nitpick comments:
In `@web/default/src/features/system-settings/api.test.ts`:
- Around line 1-54: Replace the node:test and node:assert/strict imports in the
system option update test with Vitest APIs, using vi.spyOn(api, 'put') to mock
requests and restore the spy through Vitest cleanup rather than manual
reassignment and try/finally. Preserve the existing success and rejection
assertions, following the style of update-option-notification.test.ts.
In
`@web/default/src/features/system-settings/hooks/update-option-notification.test.ts`:
- Around line 19-20: Replace the node:test and node:assert imports in the
update-option notification tests with Vitest APIs, using describe, test, expect,
beforeEach, and vi as needed. Mock the sonner module with vi.mock('sonner'),
reset mocks between tests, and assert toast.success or toast.error call
arguments instead of relying on toast.getHistory().
In
`@web/default/src/features/system-settings/hooks/update-option-notification.ts`:
- Around line 19-20: Replace the default i18next import with the named t import
in the update-option notification module, then update every i18next.t(...) call
to use t(...), preserving the existing translation keys and behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f193d15b-6a8a-4989-8214-0fe012e22fe2
📒 Files selected for processing (11)
web/default/src/features/system-settings/api.test.tsweb/default/src/features/system-settings/api.tsweb/default/src/features/system-settings/content/announcements-section.tsxweb/default/src/features/system-settings/content/api-info-section.tsxweb/default/src/features/system-settings/content/faq-section.tsxweb/default/src/features/system-settings/content/uptime-kuma-section.tsxweb/default/src/features/system-settings/general/channel-affinity/index.tsxweb/default/src/features/system-settings/hooks/update-option-notification.test.tsweb/default/src/features/system-settings/hooks/update-option-notification.tsweb/default/src/features/system-settings/hooks/use-update-option.tsweb/default/src/features/system-settings/integrations/payment-settings-section.tsx
| notification: { | ||
| error: t('Failed to save API info'), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a success notification to handleSaveAll for consistency.
The notification object only includes error — omitting success means showUpdateOptionSuccess will display the generic fallback "Setting updated successfully" toast, since undefined !== false. Both the announcements and FAQ sections provide specific success messages for their save flows. Add either a specific message or success: false if the toast should be suppressed.
Proposed fix: add specific success message
notification: {
+ success: t('API info saved successfully'),
error: t('Failed to save API info'),
},Or, to suppress the success toast entirely:
notification: {
+ success: false,
error: t('Failed to save API info'),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| notification: { | |
| error: t('Failed to save API info'), | |
| }, | |
| notification: { | |
| success: t('API info saved successfully'), | |
| error: t('Failed to save API info'), | |
| }, |
🤖 Prompt for 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.
In `@web/default/src/features/system-settings/content/api-info-section.tsx` around
lines 247 - 249, Add a success value to the notification object used by
handleSaveAll in the API info save flow, using a specific translated success
message consistent with the announcements and FAQ sections, or set success:
false if no success toast is desired.
1 participant
Important
📝 变更描述 / Description
修复新版系统设置页面保存配置时重复显示提示的问题。
系统设置表单会逐项提交发生变化的配置。此前每次更新都会独立显示成功提示；部分页面还会额外显示自己的提示，错误响应也可能同时经过 Axios 拦截器和 mutation 处理，因此一次保存可能产生多条重复通知。
本次调整包括：
AI-assisted disclosure: 本变更由 AI 辅助实现，并已完成本地代码检查和测试。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
bun test: 37 passed, 0 failedbun run typecheck: passedoxlint: passedoxfmt --check: passedbun run build: passed仓库全量 lint、format 和 copyright 检查仍受
main上已有的无关问题影响，本 PR 涉及文件均已通过对应检查。Summary by CodeRabbit