feat(show-notification): support inlineLink #1483
Conversation
概述该变更为通知系统引入了内联链接功能，允许在通知消息中添加可点击的链接。新增了 变更
详细解析接口变更
渲染逻辑
测试
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (8)
- bricks/basic/package.json: Language not supported
- bricks/basic/src/alert-notifications/styles.shadow.css: Language not supported
- bricks/basic/src/bootstrap.ts: Evaluated as low risk
- bricks/basic/src/data-providers/show-notification/show-notification.tsx: Evaluated as low risk
- bricks/basic/src/alert-notifications/interfaces.ts: Evaluated as low risk
- bricks/basic/src/data-providers/show-notification/show-notification.spec.tsx: Evaluated as low risk
- bricks/basic/src/alert-notifications/index.tsx: Evaluated as low risk
- bricks/basic/src/alert-notifications/SpeechNotifications.ts: Evaluated as low risk
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1483 +/- ##
=======================================
Coverage 90.47% 90.47%
=======================================
Files 124 124
Lines 2762 2762
Branches 379 379
=======================================
Hits 2499 2499
Misses 169 169
Partials 94 94
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🚀 Deployed on https://docs-preview-1483--next-bricks.netlify.app |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
weareoutman
force-pushed
the
steve/alert
branch
from
77e4a23 to
4ddf883
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
bricks/basic/src/alert-notifications/index.ts (2)
9-11: 建议重构服务初始化逻辑建议将服务初始化逻辑移至单独的方法中，以提高可测试性和代码组织性。这样可以更好地处理用户认证状态的变化。
-const { username } = auth.getAuth(); -// istanbul ignore next -const alertService = username ? new AlertService(username) : null; +class AlertNotifications extends NextElement { + private alertService: AlertService | null = null; + + private initializeService(): void { + const { username } = auth.getAuth(); + if (username) { + this.alertService = new AlertService(username); + } + }
21-28: 建议增强生命周期方法的健壮性建议添加以下增强：
- 为订阅操作添加错误处理
- 添加日志记录以便调试
connectedCallback() { super._markConnectedCallbackCalled(); - alertService?.subscribe(); + try { + alertService?.subscribe(); + console.debug("[alert-notifications] Successfully subscribed to alerts"); + } catch (error) { + console.error("[alert-notifications] Failed to subscribe to alerts:", error); + } } disconnectedCallback() { - alertService?.unsubscribe(); + try { + alertService?.unsubscribe(); + console.debug("[alert-notifications] Successfully unsubscribed from alerts"); + } catch (error) { + console.error("[alert-notifications] Failed to unsubscribe from alerts:", error); + } }bricks/basic/src/alert-notifications/AlertService.ts (2)
82-86: 防止内存泄漏
subscribe方法在重复调用时可能导致重复订阅。建议在#enable方法中保存messageDispatcher.subscribe的返回值并在#disable中清理。#enable() { if (!this.#active) { - messageDispatcher.subscribe(CHANNEL, { + const disposeSubscription = messageDispatcher.subscribe(CHANNEL, { system: "msgsender", topic: `monitor.alert.notify.${this.#username}`, }); this.#disposeOnMessage = messageDispatcher.onMessage( CHANNEL, this.#callback ); + this.#disposeSubscription = disposeSubscription; this.#active = true; } }
66-71: 语音通知的可访问性考虑语音通知功能需要考虑以下几点：
- 是否需要提供音量控制
- 是否需要提供语速控制
- 是否需要支持多语言
bricks/basic/src/alert-notifications/AlertService.spec.ts (1)
27-33: 补充错误处理测试建议添加测试用例验证
showNotification失败时的错误处理逻辑。it("should handle notification errors gracefully", async () => { showNotification.mockRejectedValueOnce(new Error("测试错误")); // ... 测试步骤 expect(console.error).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bricks/basic/deploy-default/package.conf.yaml(1 hunks)bricks/basic/src/alert-notifications/AlertBuffer.spec.ts(1 hunks)bricks/basic/src/alert-notifications/AlertBuffer.ts(1 hunks)bricks/basic/src/alert-notifications/AlertService.spec.ts(1 hunks)bricks/basic/src/alert-notifications/AlertService.ts(1 hunks)bricks/basic/src/alert-notifications/index.spec.ts(1 hunks)bricks/basic/src/alert-notifications/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bricks/basic/src/alert-notifications/index.spec.ts
- bricks/basic/src/alert-notifications/AlertBuffer.spec.ts
- bricks/basic/src/alert-notifications/AlertBuffer.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
bricks/basic/src/alert-notifications/AlertService.ts
[warning] 59-59: bricks/basic/src/alert-notifications/AlertService.ts#L59
Added line #L59 was not covered by tests
bricks/basic/src/alert-notifications/index.ts
[warning] 30-30: bricks/basic/src/alert-notifications/index.ts#L30
Added line #L30 was not covered by tests
🔇 Additional comments (6)
bricks/basic/src/alert-notifications/index.ts (2)
16-20: 代码实现规范，符合最佳实践组件定义遵循了命名空间规范，样式引入方式正确，继承关系合理。
30-32: 建议完善空方法的文档说明和测试覆盖
- 建议添加详细的文档注释，说明为什么该方法不需要实现。
- 即使是空方法，也应该添加相应的单元测试以提高测试覆盖率。
+/** + * 该组件不需要渲染UI元素，仅负责处理告警通知的订阅。 + * @override + */ _render() { // Do nothing }建议添加以下测试用例：
it("should not render any UI elements", () => { const element = new AlertNotifications(); element._render(); expect(element.shadowRoot?.children.length).toBe(0); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-30: bricks/basic/src/alert-notifications/index.ts#L30
Added line #L30 was not covered by testsbricks/basic/deploy-default/package.conf.yaml (1)
5-5: 验证依赖版本更新的兼容性依赖版本从
^2.90.33 || ^3.8.38更新到^2.90.33 || ^3.19.23，请确认：
- 新版本是否包含不兼容的更改
- 是否需要更新其他相关依赖
bricks/basic/src/alert-notifications/AlertService.ts (1)
59-61: 改进错误处理机制在显示通知时捕获到的错误被静默忽略，这可能导致问题难以排查。建议至少记录错误信息。
.catch((error) => { - /* Do nothing */ + console.error('显示通知时发生错误：', error); })🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 59-59: bricks/basic/src/alert-notifications/AlertService.ts#L59
Added line #L59 was not covered by testsbricks/basic/src/alert-notifications/AlertService.spec.ts (2)
71-107: 改进时间相关的测试使用固定时间戳进行测试可能导致测试不稳定。建议使用 Jest 的时间模拟功能。
- time: Math.round(+new Date("2025-01-21") / 1000), + time: Math.round(Date.UTC(2025, 0, 21, 8, 0, 0) / 1000),
109-145: 补充语音通知测试语音通知的测试用例需要验证
SpeechNotifications的调用。expect(mockSpeechNotifications.play).toHaveBeenCalledWith( "2025-01-21 08:00:00【Warning】test target-test origin content" );
weareoutman
force-pushed
the
steve/alert
branch
from
4ddf883 to
0eda238
Compare
weareoutman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bricks/basic/src/data-providers/show-notification/show-notification.tsx (1)
251-261: 建议将样式迁移到 CSS 模块中当前实现逻辑清晰，但建议将内联样式
marginLeft: "0.5em"移至 CSS 模块中，以保持样式的一致性和可维护性。- style={{ marginLeft: "0.5em" }} + className={styles.inlineLink}请在
notification.module.css中添加：.inlineLink { margin-left: 0.5em; }🧰 Tools
🪛 Biome (1.9.4)
[error] 255-255: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bricks/basic/src/data-providers/show-notification/show-notification.spec.tsx(1 hunks)bricks/basic/src/data-providers/show-notification/show-notification.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bricks/basic/src/data-providers/show-notification/show-notification.spec.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
bricks/basic/src/data-providers/show-notification/show-notification.tsx
[error] 255-255: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build docs (20.x)
- GitHub Check: Size check (20.x)
🔇 Additional comments (2)
bricks/basic/src/data-providers/show-notification/show-notification.tsx (2)
21-21: 接口设计清晰明确！新增的
inlineLink接口设计合理，类型定义完整，并且与现有代码风格保持一致。使用 lodash 的pick函数来提取特定属性也是一个很好的做法。Also applies to: 41-46
251-261: HTML 内容安全处理符合最佳实践虽然静态分析工具标记了
dangerouslySetInnerHTML的使用，但代码中已经正确使用 DOMPurify 进行了安全处理，这是处理 HTML 内容的最佳实践。🧰 Tools
🪛 Biome (1.9.4)
[error] 255-255: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
依赖检查
组件之间的依赖声明，是微服务组件架构下的重要信息，请确保其正确性。
请勾选以下两组选项其中之一：
或者：
提交信息检查
Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG，请检查工作内容与提交信息是否相符，并在以下每组选项中都依次确认。
破坏性变更：
feat作为提交类型。BREAKING CHANGE: 你的变更说明。新特性：
feat作为提交类型。问题修复：
fix作为提交类型。杂项工作：
即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动，例如修改注释、测试用例、开发文档等：
chore,docs,test等作为提交类型。Summary by CodeRabbit
新功能
测试