feat(notify-rules): redesign form with section cards, filter collapse, auto-name and mock test - #2195
Merged
Conversation
…, auto-name and mock test
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe notification rule form is reorganized into collapsible sections with automatic naming, validation-driven expansion, enhanced filter summaries, history/mock testing, pipeline configuration integration, and expanded localized guidance. ChangesNotification rule form
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestButton
participant EventsTable
participant getEvents
participant notifyRuleTest
TestButton->>EventsTable: request history events
EventsTable->>getEvents: fetch events and total
getEvents-->>EventsTable: return events
EventsTable-->>TestButton: report historyTotal
TestButton->>notifyRuleTest: submit history or mock test
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
# Conflicts: # src/pages/alertRules/FormNG/components/SectionCard/index.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pages/Form/TestButton.tsx`:
- Around line 51-54: Update the response formatting logic in TestButton to
operate on the payload rather than the already-parsed response object: use
res.dat as the value passed to JSON parsing/stringification, preserving
pretty-printed output when the payload is valid JSON and the existing fallback
behavior otherwise.
- Line 60: Update the successful-test path in handleTest to call handleClose
instead of only setVisible(false), ensuring mode, selectedEventIds, and
historyTotal are reset before the modal reopens. Preserve the existing success
behavior while reusing the cancel cleanup path.
- Around line 47-61: Add rejection handling to the notifyRuleTest promise chain
in TestButton, using the component’s existing error-reporting pattern to surface
failed test requests and close or reset the modal consistently. Preserve the
current success response formatting and Modal.info behavior, while ensuring
rejected requests are not left unhandled.
🪄 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: c9c0075c-70cc-41a7-b030-d274e87d412b
📒 Files selected for processing (13)
src/pages/alertRules/FormNG/components/SectionCard/index.tsxsrc/pages/eventPipeline/pages/Form/TestModal/EventsTable.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/notificationRules/pages/Form/EventPipelineConfigs/index.tsxsrc/pages/notificationRules/pages/Form/RuleConfig.tsxsrc/pages/notificationRules/pages/Form/TestButton.tsxsrc/pages/notificationRules/pages/Form/index.tsxsrc/pages/notificationRules/services.tssrc/pages/notificationRules/types.ts
Comment on lines
+47
to
+61
| notifyRuleTest({ | ||
| ...(mode === 'mock' ? { use_mock_event: true } : { event_ids: selectedEventIds }), | ||
| notify_config: buildNotifyConfigPayload(), | ||
| }).then((res) => { | ||
| let msg = res.dat; | ||
| try { | ||
| msg = JSON.stringify(JSON.parse(res), null, 2); | ||
| } catch (e) {} | ||
|
|
||
| Modal.info({ | ||
| title: t('notification_configuration.run_test_request_result'), | ||
| content: <div>{msg}</div>, | ||
| }); | ||
| setVisible(false); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling to the notifyRuleTest call.
The .then(...) chain has no .catch; a failed test request rejects unhandled and the modal never closes or surfaces the error. As per coding guidelines, "Async requests must have error handling (try/catch, .catch, onError, etc.) consistent with existing patterns".
🛡️ Proposed fix
}).then((res) => {
let msg = res.dat;
try {
msg = JSON.stringify(JSON.parse(res), null, 2);
} catch (e) {}
Modal.info({
title: t('notification_configuration.run_test_request_result'),
content: <div>{msg}</div>,
});
setVisible(false);
- });
+ }).catch(() => {
+ // surface/handle request failure consistent with existing patterns
+ });📝 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
| notifyRuleTest({ | |
| ...(mode === 'mock' ? { use_mock_event: true } : { event_ids: selectedEventIds }), | |
| notify_config: buildNotifyConfigPayload(), | |
| }).then((res) => { | |
| let msg = res.dat; | |
| try { | |
| msg = JSON.stringify(JSON.parse(res), null, 2); | |
| } catch (e) {} | |
| Modal.info({ | |
| title: t('notification_configuration.run_test_request_result'), | |
| content: <div>{msg}</div>, | |
| }); | |
| setVisible(false); | |
| }); | |
| notifyRuleTest({ | |
| ...(mode === 'mock' ? { use_mock_event: true } : { event_ids: selectedEventIds }), | |
| notify_config: buildNotifyConfigPayload(), | |
| }).then((res) => { | |
| let msg = res.dat; | |
| try { | |
| msg = JSON.stringify(JSON.parse(res), null, 2); | |
| } catch (e) {} | |
| Modal.info({ | |
| title: t('notification_configuration.run_test_request_result'), | |
| content: <div>{msg}</div>, | |
| }); | |
| setVisible(false); | |
| }).catch(() => { | |
| // surface/handle request failure consistent with existing patterns | |
| }); |
🤖 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/pages/Form/TestButton.tsx` around lines 47 - 61,
Add rejection handling to the notifyRuleTest promise chain in TestButton, using
the component’s existing error-reporting pattern to surface failed test requests
and close or reset the modal consistently. Preserve the current success response
formatting and Modal.info behavior, while ensuring rejected requests are not
left unhandled.
Source: Coding guidelines
Comment on lines
+51
to
+54
| let msg = res.dat; | ||
| try { | ||
| msg = JSON.stringify(JSON.parse(res), null, 2); | ||
| } catch (e) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
JSON.parse(res) parses the whole response, not the payload.
res is the already-parsed umi-request response object, so JSON.parse(res) throws and is swallowed by the empty catch — the pretty-print branch is effectively dead and msg always stays res.dat. This is likely meant to be JSON.parse(res.dat) (or JSON.stringify(res.dat, null, 2)).
🤖 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/pages/Form/TestButton.tsx` around lines 51 - 54,
Update the response formatting logic in TestButton to operate on the payload
rather than the already-parsed response object: use res.dat as the value passed
to JSON parsing/stringification, preserving pretty-printed output when the
payload is valid JSON and the existing fallback behavior otherwise.
…annel fetch error
SectionCard 原本靠 item + index 两个 props 传入，调用点硬编码下标， 分区顺序一调整就要多处同步修改（advanced 分区渲染在 notify 之后、 序号却是 7，页面显示为 4→7→5→6 就是漏改导致）。 - 新增 SectionsProvider/SectionsConfig：Record<key, config> 的书写顺序 即展示顺序与序号，可嵌套以便 plus 侧动态分区接续编号 - SectionCard 只接收 sectionKey，标题/描述/序号均按 key 取值 - 各调用点去掉 item/index/advancedItem/baseIndex 等下标相关 props - 告警规则页 advanced 分区顺序移到 notify 之后，编号与渲染顺序对齐
高级配置原先由 Notify 组件内部紧跟通知配置渲染，位置与配置表顺序耦合。 现抽出 Notify/Advanced 包一层取 FormNGData，在事件处理配置之后渲染， 分区配置表中 advanced 也相应放到最后，编号变为最后一位。
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/notificationRules/pages/Form/EventPipelineConfigs/index.tsx (1)
186-191: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDisable refresh while a request is in flight.
The button only spins while
loadingis true, so rapid clicks can issue concurrent requests. Out-of-order responses may overwriteeventPipelineListwith stale data. Adddisabled={loading}or guardfetchDataagainst overlapping requests.🤖 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/pages/Form/EventPipelineConfigs/index.tsx` around lines 186 - 191, Update the refresh Button in the EventPipelineConfigs component to prevent clicks while loading is true by adding the disabled state, or enforce the same guard at the start of fetchData. Preserve the existing refresh behavior once the in-flight request completes.
🤖 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/alertRules/FormNG/index.tsx`:
- Around line 277-319: Update the core-only collapse handler in the Button using
setSectionCollapsed to also emit the shared toggle signal via
setToggleAllSignal, ensuring pipeline, notify, effective, and advanced collapse
in the same way as the existing signal-driven panels. Preserve the current
sectionCollapsed updates and use the matching collapse action.
---
Outside diff comments:
In `@src/pages/notificationRules/pages/Form/EventPipelineConfigs/index.tsx`:
- Around line 186-191: Update the refresh Button in the EventPipelineConfigs
component to prevent clicks while loading is true by adding the disabled state,
or enforce the same guard at the start of fetchData. Preserve the existing
refresh behavior once the in-flight request completes.
🪄 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: 5f265213-b121-4e4b-a3ca-b9feaaf6359e
📒 Files selected for processing (9)
src/pages/alertRules/FormNG/Effective/index.tsxsrc/pages/alertRules/FormNG/Notify/index.tsxsrc/pages/alertRules/FormNG/PipelineConfigsNG/index.tsxsrc/pages/alertRules/FormNG/components/SectionCard/context.tsxsrc/pages/alertRules/FormNG/components/SectionCard/index.tsxsrc/pages/alertRules/FormNG/index.tsxsrc/pages/notificationRules/pages/Form/EventPipelineConfigs/index.tsxsrc/pages/notificationRules/pages/Form/TestButton.tsxsrc/pages/notificationRules/pages/Form/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/notificationRules/pages/Form/TestButton.tsx
- src/pages/notificationRules/pages/Form/index.tsx
Comment on lines
+277
to
+319
| <Button | ||
| onClick={() => { | ||
| scroll.setSectionCollapsed((prev) => ({ | ||
| ...prev, | ||
| basic: false, | ||
| datasource: false, | ||
| rule: false, | ||
| pipeline: true, | ||
| notify: true, | ||
| effective: true, | ||
| advanced: true, | ||
| })); | ||
| }} | ||
| className='flex items-center gap-1' | ||
| size='small' | ||
| icon={<Sparkles size={12} className='text-error' />} | ||
| > | ||
| {t('form_ng.collapse_core_only')} | ||
| </Button> | ||
| {(() => { | ||
| const visibleKeys = _.keys(sections); | ||
| const allExpanded = visibleKeys.every((k) => scroll.sectionCollapsed[k] === false); | ||
| return ( | ||
| <Button | ||
| onClick={() => { | ||
| scroll.setSectionCollapsed((prev) => { | ||
| const anyCollapsed = visibleKeys.some((k) => prev[k] === true); | ||
| const next = {}; | ||
| for (const k of visibleKeys) { | ||
| next[k] = anyCollapsed ? false : true; | ||
| } | ||
| return { ...prev, ...next }; | ||
| }); | ||
| scroll.setToggleAllSignal({ action: allExpanded ? 'collapse' : 'expand', ts: Date.now() }); | ||
| }} | ||
| className='flex items-center gap-1' | ||
| size='small' | ||
| icon={allExpanded ? <ChevronsDownUp size={12} /> : <ChevronsUpDown size={12} />} | ||
| > | ||
| {allExpanded ? t('form_ng.collapse_collapse_all') : t('form_ng.collapse_expand_all')} | ||
| </Button> | ||
| ); | ||
| })()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'useScrollSync' src/pages/alertRules/FormNG/utils
fd 'useScrollSync' src/pages/alertRules/FormNG/utils --exec cat -n {}Repository: n9e/fe
Length of output: 5406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first, then inspect the smallest useful slices.
fd -a 'SectionCard' src/pages/alertRules/FormNG/components
fd -a 'Notify' src/pages/alertRules/FormNG/components
fd -a 'Effective' src/pages/alertRules/FormNG/components
fd -a 'Pipeline' src/pages/alertRules/FormNG/components
fd -a 'FormNG' src/pages/alertRules/FormNG/index.tsx
printf '\n--- SectionCard outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/SectionCard.tsx --view expanded
printf '\n--- Notify outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/NotifyConfigsNG.tsx --view expanded
printf '\n--- Effective outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/EffectiveConfigsNG.tsx --view expanded
printf '\n--- Pipeline outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/PipelineConfigsNG.tsx --view expanded
printf '\n--- FormNG button region ---\n'
sed -n '250,340p' src/pages/alertRules/FormNG/index.tsx
printf '\n--- SectionCard region ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/SectionCard.tsx
printf '\n--- Notify region ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/NotifyConfigsNG.tsx
printf '\n--- Effective region ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/EffectiveConfigsNG.tsx
printf '\n--- Pipeline region ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/PipelineConfigsNG.tsxRepository: n9e/fe
Length of output: 340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the first pass is too broad, inspect the exact symbols that control collapsed state.
rg -n "toggleAllSignal|expandSignal|collapsed|sectionCollapsed|setSectionCollapsed" src/pages/alertRules/FormNG/components src/pages/alertRules/FormNG/index.tsx src/pages/alertRules/FormNG/utils/useScrollSync.tsRepository: n9e/fe
Length of output: 5370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused read-only probe for the collapse-state flow.
python3 - <<'PY'
from pathlib import Path
files = [
Path('src/pages/alertRules/FormNG/index.tsx'),
Path('src/pages/alertRules/FormNG/components/SectionCard.tsx'),
Path('src/pages/alertRules/FormNG/components/NotifyConfigsNG.tsx'),
Path('src/pages/alertRules/FormNG/components/EffectiveConfigsNG.tsx'),
Path('src/pages/alertRules/FormNG/components/PipelineConfigsNG.tsx'),
Path('src/pages/alertRules/FormNG/utils/useScrollSync.ts'),
]
for p in files:
print(f"\n=== {p} ===")
if not p.exists():
print("MISSING")
continue
text = p.read_text()
for needle in ["toggleAllSignal", "expandSignal", "sectionCollapsed", "setSectionCollapsed", "collapsed", "onCollapse"]:
if needle in text:
print(f"-- contains {needle}")
PYRepository: n9e/fe
Length of output: 852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the actual component files and inspect the collapse-state flow in them.
fd -a 'index.tsx' src/pages/alertRules/FormNG/components/SectionCard
fd -a 'NotifyConfigsNG.tsx' src/pages/alertRules/FormNG/components
fd -a 'EffectiveConfigsNG.tsx' src/pages/alertRules/FormNG/components
fd -a 'PipelineConfigsNG.tsx' src/pages/alertRules/FormNG/components
fd -a 'NotifyAdvanced' src/pages/alertRules/FormNG/components
printf '\n--- SectionCard outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/SectionCard/index.tsx --view expanded
printf '\n--- Notify outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/NotifyConfigsNG/index.tsx --view expanded
printf '\n--- Effective outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/EffectiveConfigsNG/index.tsx --view expanded
printf '\n--- Pipeline outline ---\n'
ast-grep outline src/pages/alertRules/FormNG/components/PipelineConfigsNG/index.tsx --view expanded
printf '\n--- Notify component ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/NotifyConfigsNG/index.tsx
printf '\n--- Effective component ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/EffectiveConfigsNG/index.tsx
printf '\n--- Pipeline component ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/PipelineConfigsNG/index.tsx
printf '\n--- Advanced component ---\n'
sed -n '1,260p' src/pages/alertRules/FormNG/components/NotifyAdvanced/index.tsx
printf '\n--- SectionCard component ---\n'
sed -n '1,220p' src/pages/alertRules/FormNG/components/SectionCard/index.tsxRepository: n9e/fe
Length of output: 1295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Tight check for who reacts to sectionCollapsed vs the toggle signals.
rg -n "sectionCollapsed|toggleAllSignal|expandSignal|setCollapsed|collapsed=" src/pages/alertRules/FormNG/components src/pages/alertRules/FormNG/index.tsxRepository: n9e/fe
Length of output: 2506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path('src/pages/alertRules/FormNG/index.tsx'),
Path('src/pages/alertRules/FormNG/components/SectionCard/index.tsx'),
Path('src/pages/alertRules/FormNG/components/NotifyConfigsNG/index.tsx'),
Path('src/pages/alertRules/FormNG/components/EffectiveConfigsNG/index.tsx'),
Path('src/pages/alertRules/FormNG/components/PipelineConfigsNG/index.tsx'),
Path('src/pages/alertRules/FormNG/components/NotifyAdvanced/index.tsx'),
]
for p in paths:
print(f"\n=== {p} ===")
if not p.exists():
print("MISSING")
continue
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(k in line for k in ["toggleAllSignal", "expandSignal", "sectionCollapsed", "setCollapsed", "collapsed", "onCollapse"]):
print(f"{i}: {line}")
PYRepository: n9e/fe
Length of output: 3259
Route the core-only collapse through the shared signal. notify, effective, pipeline, and advanced ignore sectionCollapsed here, so this button only updates dead state for those panels. Emit the matching toggle signal as well, or move them onto the same collapse source of truth.
🤖 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/alertRules/FormNG/index.tsx` around lines 277 - 319, Update the
core-only collapse handler in the Button using setSectionCollapsed to also emit
the shared toggle signal via setToggleAllSignal, ensuring pipeline, notify,
effective, and advanced collapse in the same way as the existing signal-driven
panels. Preserve the current sectionCollapsed updates and use the matching
collapse action.
冲突来自 main 的 31b38ba：双方都把 NotifyExtraNG 从 Notify 提到了父组件。 解决方式： - 保留本分支的 sectionKey 方案（SectionCard 不再接收 item/index） - 采用 main 的 AdvancedSettingsSection 内联组件与渲染位置，删掉本分支 等价的 Notify/Advanced.tsx，仅把 advancedItem 换成按 sections.advanced 判空 - 采纳 main 对通知配置卡片的 className='mb-8'
冲突来自 main 的 d565fab：双方都在去掉 SectionCard 的硬编码序号。 main 的做法是保留 item + index，把 index 换成 sectionKeys.indexOf(item.key)， 并把 sectionKeys 数组透传到 Notify / Effective / PipelineConfigsNG / NotifyExtraNG。 本分支的 SectionsProvider 方案已覆盖同一目的：组件只传 sectionKey，标题描述与 序号都从分区配置表按 key 取，无需透传 item / index / sectionKeys，故冲突处取本 分支版本，sectionKeys 透传随之移除。
main 已由 31b38ba、d565fab53 完成同一目标（去掉硬编码序号、把 NotifyExtraNG 提到父组件），本分支的 SectionsProvider 方案与之重复， 故整体回退 alertRules/FormNG，通知规则表单改回 item + index 用法， 本 PR 只保留通知规则相关改动。 SectionCard 仅保留本 PR 需要的两处：SectionItem 支持自定义 icon、 sectionRef 改为可选（通知规则表单不做滚动同步）。
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
New Features
Documentation