refactor(subscribe): rebuild subscribe form with section cards, auto-naming and shared notification rule select - #2198
Merged
Merged
Conversation
…naming and shared notification rule select
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR introduces a reusable notification-rule selector, refactors subscription configuration into collapsible sections with summaries and automatic naming, gates rule loading by authorization, adds empty-state and scenario guidance, centralizes documentation links, and supplies localized strings. ChangesNotification rule selection
Subscription configuration
Subscription list and guidance
Subscription localization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pages/notificationRules/components/RuleDropdownSelect/index.tsx`:
- Around line 324-330: In
src/pages/notificationRules/components/RuleDropdownSelect/index.tsx lines
324-330, add rejection handling to the putNotificationRule(values) promise
chain. Also add equivalent .catch handling to the
createNotificationRules([values]) chain at lines 350-361 before its existing
.finally block, preserving the current success and cleanup behavior.
In `@src/pages/warning/subscribe/components/operateForm.tsx`:
- Around line 626-637: Move the stable key from the inner Form.Item to the outer
Row returned by fields.map, using the mapped field’s stable identifier such as
field.key. Keep the existing Form.Item name, validation rules, and remove
behavior unchanged.
🪄 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: 7cc5bfce-bab6-4674-8163-83b7f0f6b742
📒 Files selected for processing (19)
src/pages/alertRules/FormNG/Notify/index.tsxsrc/pages/alertRules/FormNG/components/SectionCard/index.tsxsrc/pages/notificationRules/components/RuleDropdownSelect/index.tsxsrc/pages/notificationRules/locale/en_US.tssrc/pages/notificationRules/locale/ja_JP.tssrc/pages/notificationRules/locale/ru_RU.tssrc/pages/notificationRules/locale/zh_CN.tssrc/pages/notificationRules/locale/zh_HK.tssrc/pages/warning/subscribe/ListNG.tsxsrc/pages/warning/subscribe/components/buildAutoName.test.tssrc/pages/warning/subscribe/components/buildAutoName.tssrc/pages/warning/subscribe/components/operateForm.tsxsrc/pages/warning/subscribe/components/utils.tssrc/pages/warning/subscribe/index.tsxsrc/pages/warning/subscribe/locale/en_US.tssrc/pages/warning/subscribe/locale/ja_JP.tssrc/pages/warning/subscribe/locale/ru_RU.tssrc/pages/warning/subscribe/locale/zh_CN.tssrc/pages/warning/subscribe/locale/zh_HK.ts
Comment on lines
324
to
330
| onOk={(values) => { | ||
| putNotificationRule(values).then(() => { | ||
| message.success(t('common:success.edit')); | ||
| handleCloseViewDrawer(); | ||
| refreshNotificationRules(); | ||
| refresh?.(); | ||
| }); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add error handling for asynchronous requests.
As per coding guidelines, all asynchronous requests must have error handling (try/catch, .catch, onError, etc.) consistent with existing patterns. These API calls are missing a .catch block to safely handle potential promise rejections.
src/pages/notificationRules/components/RuleDropdownSelect/index.tsx#L324-L330: Append.catch(console.error)or equivalent handling to theputNotificationRule(values).then(...)chain.src/pages/notificationRules/components/RuleDropdownSelect/index.tsx#L350-L361: Append.catch(...)to thecreateNotificationRules([values]).then(...)chain before the.finally(...)block.
📍 Affects 1 file
src/pages/notificationRules/components/RuleDropdownSelect/index.tsx#L324-L330(this comment)src/pages/notificationRules/components/RuleDropdownSelect/index.tsx#L350-L361
🤖 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 `@src/pages/notificationRules/components/RuleDropdownSelect/index.tsx` around
lines 324 - 330, In
src/pages/notificationRules/components/RuleDropdownSelect/index.tsx lines
324-330, add rejection handling to the putNotificationRule(values) promise
chain. Also add equivalent .catch handling to the
createNotificationRules([values]) chain at lines 350-361 before its existing
.finally block, preserving the current success and cleanup behavior.
Source: Coding guidelines
Comment on lines
+626
to
+637
| {fields.map((field, index) => ( | ||
| <Row gutter={10}> | ||
| <Col flex='auto'> | ||
| <Form.Item name={[field.name]} key={index} rules={[{ required: true, message: t('webhooks_msg') }]}> | ||
| <Input /> | ||
| </Form.Item> | ||
| </Col> | ||
| <Col flex='32px'> | ||
| <MinusCircleOutlined style={{ marginTop: '8px' }} onClick={() => remove(field.name)} /> | ||
| </Col> | ||
| </Row> | ||
| ))} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a stable key to the mapped <Row>.
The top-level element returned from fields.map is <Row>, but the key is placed on the inner <Form.Item> instead. React needs the key on the outermost mapped node, otherwise rows can be mismatched to data on reorder/removal.
🐛 Proposed fix
- {fields.map((field, index) => (
- <Row gutter={10}>
+ {fields.map((field, index) => (
+ <Row gutter={10} key={field.key}>
<Col flex='auto'>
- <Form.Item name={[field.name]} key={index} rules={[{ required: true, message: t('webhooks_msg') }]}>
+ <Form.Item name={[field.name]} rules={[{ required: true, message: t('webhooks_msg') }]}>
<Input />
</Form.Item>📝 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.
Suggested change
| {fields.map((field, index) => ( | |
| <Row gutter={10}> | |
| <Col flex='auto'> | |
| <Form.Item name={[field.name]} key={index} rules={[{ required: true, message: t('webhooks_msg') }]}> | |
| <Input /> | |
| </Form.Item> | |
| </Col> | |
| <Col flex='32px'> | |
| <MinusCircleOutlined style={{ marginTop: '8px' }} onClick={() => remove(field.name)} /> | |
| </Col> | |
| </Row> | |
| ))} | |
| {fields.map((field, index) => ( | |
| <Row gutter={10} key={field.key}> | |
| <Col flex='auto'> | |
| <Form.Item name={[field.name]} rules={[{ required: true, message: t('webhooks_msg') }]}> | |
| <Input /> | |
| </Form.Item> | |
| </Col> | |
| <Col flex='32px'> | |
| <MinusCircleOutlined style={{ marginTop: '8px' }} onClick={() => remove(field.name)} /> | |
| </Col> | |
| </Row> | |
| ))} |
🧰 Tools
🪛 React Doctor (0.7.6)
[error] 627-627: Your users can see the wrong data when this list reorders.
Add a stable key prop so React can keep list items matched to the right data when the list changes.
(jsx-key)
🤖 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 `@src/pages/warning/subscribe/components/operateForm.tsx` around lines 626 -
637, Move the stable key from the inner Form.Item to the outer Row returned by
fields.map, using the mapped field’s stable identifier such as field.key. Keep
the existing Form.Item name, validation rules, and remove behavior unchanged.
Source: Linters/SAST tools
…e to subscribe rules
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/warning/subscribe/components/operateForm.tsx (1)
262-264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
severitiesin thehasAnyFiltercheck.If a user unchecks some severities (e.g., leaving only Critical
[1]), the subscription no longer hits "all alert events". However,hasAnyFilterdoes not checkseveritiesValue, so the warning about hitting all events might incorrectly display when only specific severities are selected.🐛 Proposed fix
- const hasAnyFilter = !!cate || !!filterCounts.rules || !!filterCounts.busiGroups || !!filterCounts.tags || !!forDurationValue; + const hasAnyFilter = !!cate || !!filterCounts.rules || !!filterCounts.busiGroups || !!filterCounts.tags || !!forDurationValue || _.size(severitiesValue) < 3;🤖 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 `@src/pages/warning/subscribe/components/operateForm.tsx` around lines 262 - 264, Update the hasAnyFilter calculation in operateForm so it also evaluates severitiesValue, preventing the all-events warning when the user selects specific severities. Preserve the existing checks for category, rules, business groups, tags, and duration.
🧹 Nitpick comments (1)
src/pages/warning/subscribe/components/operateForm.tsx (1)
512-514: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
field.keyas thekeyprop when mappingForm.Listfields.Using
indexas a key in dynamic lists can cause React to mismatch component state when items are added, removed, or reordered. Ant Design provides a stablefield.keyfor this purpose.♻️ Proposed fix
- {fields.map((field, index) => ( - <BusiGroupsTagItem key={index} field={field} fields={fields} index={index} remove={remove} add={add} form={form} /> - ))} + {fields.map((field, index) => ( + <BusiGroupsTagItem key={field.key} field={field} fields={fields} index={index} remove={remove} add={add} form={form} /> + ))}🤖 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 `@src/pages/warning/subscribe/components/operateForm.tsx` around lines 512 - 514, Update the fields.map rendering in the BusiGroupsTagItem list to use each Form.List field’s stable field.key for the React key prop instead of the array index, while leaving the remaining props unchanged.
🤖 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.
Outside diff comments:
In `@src/pages/warning/subscribe/components/operateForm.tsx`:
- Around line 262-264: Update the hasAnyFilter calculation in operateForm so it
also evaluates severitiesValue, preventing the all-events warning when the user
selects specific severities. Preserve the existing checks for category, rules,
business groups, tags, and duration.
---
Nitpick comments:
In `@src/pages/warning/subscribe/components/operateForm.tsx`:
- Around line 512-514: Update the fields.map rendering in the BusiGroupsTagItem
list to use each Form.List field’s stable field.key for the React key prop
instead of the array index, while leaving the remaining props unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c870022-f099-47d1-b454-2c57d36b98fb
📒 Files selected for processing (14)
src/components/EmptyGuide/index.tsxsrc/pages/warning/subscribe/ListNG.tsxsrc/pages/warning/subscribe/add.tsxsrc/pages/warning/subscribe/components/ScenarioList.tsxsrc/pages/warning/subscribe/components/ScenarioTips.tsxsrc/pages/warning/subscribe/components/operateForm.tsxsrc/pages/warning/subscribe/constants.tssrc/pages/warning/subscribe/edit.tsxsrc/pages/warning/subscribe/index.tsxsrc/pages/warning/subscribe/locale/en_US.tssrc/pages/warning/subscribe/locale/ja_JP.tssrc/pages/warning/subscribe/locale/ru_RU.tssrc/pages/warning/subscribe/locale/zh_CN.tssrc/pages/warning/subscribe/locale/zh_HK.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/pages/warning/subscribe/index.tsx
- src/pages/warning/subscribe/locale/ru_RU.ts
- src/pages/warning/subscribe/locale/ja_JP.ts
- src/pages/warning/subscribe/locale/zh_HK.ts
- src/pages/warning/subscribe/locale/en_US.ts
- src/pages/warning/subscribe/locale/zh_CN.ts
- src/pages/warning/subscribe/ListNG.tsx
…ped error sections - 告警级别未全选也算筛选条件，避免只收敛级别时仍提示「将命中所有告警事件」 - 校验失败时若出错字段未登记在分区映射表里，兜底展开全部分区， 防止错误项藏在 display:none 的分区中导致提交像没反应 - 把散落的 [1, 2, 3] 收敛为 SEVERITIES 常量
# Conflicts: # src/pages/alertRules/FormNG/components/SectionCard/index.tsx
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit