feat(renderer): enhance notification system with customizable options - #216
feat(renderer): enhance notification system with customizable options#216lhuans wants to merge 1 commit into
Conversation
WalkthroughThe renderer now supports typed notification settings, runtime settings accessors, configurable notification handlers, browser toast rendering with dismissal behavior, and console fallbacks. Notification-related types are also exposed through the public API. ChangesNotification system
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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 `@projects/tiny-schema-renderer-ng/projects/renderer/src/renderer-main.ts`:
- Line 70: Remove the setRendererSettings call from the RendererMain constructor
or initialization path so component instantiation cannot overwrite module-level
currentSettings. Move renderer settings initialization to a single
application-level mechanism such as APP_INITIALIZER or a core singleton service;
if settings must differ per RendererMain instance, update notification dispatch
and parser utilities to receive instance-specific context instead of using the
global getter.
🪄 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: cf4976fc-6c99-4661-8cee-10eef8914039
📒 Files selected for processing (4)
projects/tiny-schema-renderer-ng/projects/renderer/src/parser/notify.tsprojects/tiny-schema-renderer-ng/projects/renderer/src/public-api.tsprojects/tiny-schema-renderer-ng/projects/renderer/src/renderer-main.tsprojects/tiny-schema-renderer-ng/projects/renderer/src/renderer-settings.ts
| ) { | ||
| this.cssScopeId = `data-schema-${Math.random().toString(36).slice(2, 8)}`; | ||
| this.contextService.setMaterials(this.rendererSettings?.materials ?? {}); | ||
| setRendererSettings(this.rendererSettings); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Race condition on global renderer settings.
Updating a module-level singleton (currentSettings) from a component constructor causes a "last instance wins" race condition. If multiple RendererMain components are instantiated in the application (e.g., one with a custom notify handler and another without), the last instantiated component will immediately overwrite the settings for all instances globally.
If Notify requires access to custom handlers synchronously, consider initializing these global settings once (e.g., via Angular's APP_INITIALIZER or a core singleton service) instead of mutating global state on component instantiation. Alternatively, if instance-level notification configuration is necessary, the parsers and utilities triggering the notifications will need context-aware dispatching rather than relying on a static module-level getter.
🤖 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 `@projects/tiny-schema-renderer-ng/projects/renderer/src/renderer-main.ts` at
line 70, Remove the setRendererSettings call from the RendererMain constructor
or initialization path so component instantiation cannot overwrite module-level
currentSettings. Move renderer settings initialization to a single
application-level mechanism such as APP_INITIALIZER or a core singleton service;
if settings must differ per RendererMain instance, update notification dispatch
and parser utilities to receive instance-specific context instead of using the
global getter.
|
|
||
| export const RENDERER_SETTINGS = new InjectionToken<IRendererSettings>('RENDERER_SETTINGS'); | ||
|
|
||
| let currentSettings: IRendererSettings = {}; |
There was a problem hiding this comment.
遗留：单例模式存在不同实例之间相互影响的问题，待优化方案
2 participants
渲染器 notify 去除组件库依赖,保持一致性
Summary by CodeRabbit