Conversation
WalkthroughReplaces debounced value-change emissions in the rich-text input with a blur- and lifecycle-driven save flow that tracks toolbar interaction; adds ViewChild, AfterViewInit, OnDestroy hooks, and extra inputs. Two gallery editors now debounce file-clicks (200ms) using Subjects and use DestroyRef/takeUntilDestroyed for lifecycle-bound subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorTemplate as RichText Template
participant AngularEditor as AngularEditorComponent
participant RichTextComp as RichTextInputComponent
User->>EditorTemplate: Focus / Type
EditorTemplate->>AngularEditor: user input
Note over AngularEditor,RichTextComp: While typing — no immediate save
User->>AngularEditor: Click toolbar
AngularEditor->>RichTextComp: toolbar mousedown (sets toolbar flag)
User->>AngularEditor: Click outside (blur)
AngularEditor->>RichTextComp: blur event
alt toolbar was active
RichTextComp->>RichTextComp: skip save (toolbar interaction)
else toolbar not active
RichTextComp->>RichTextComp: saveContent() -> emit `update` if changed
end
RichTextComp->>RichTextComp: onDestroy -> flush final save if needed
sequenceDiagram
participant User
participant GalleryTemplate as Gallery Template
participant GalleryComp as GalleryEditorComponent
participant FileSubject as fileSelectionSubject$
User->>GalleryTemplate: Click file
GalleryTemplate->>GalleryComp: onFileClick(file)
GalleryComp->>FileSubject: fileSelectionSubject$.next(file)
rect rgba(200, 220, 255, 0.25)
Note over FileSubject,GalleryComp: 200ms debounce window
end
FileSubject->>GalleryComp: Debounced emission
GalleryComp->>GalleryComp: setSelectedFile(file)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/src/app/inputs/rich-text-input.component.ts (1)
37-46: Breaking change:inputFocusoutput removed but still used by parent component.The
inputFocusoutput was removed, butsetting.component.tsstill binds to it with(inputFocus)="updateComponentFocus($event)". This will cause runtime errors or binding warnings.Apply this diff to restore the output and emit focus events:
@Input() value: string; @Output() update = new EventEmitter<string>(); +@Output() inputFocus = new EventEmitter<boolean>();Then emit focus events in ngAfterViewInit:
// Save only when actually leaving the editor this.blurListener = () => { if (!this.isClickingToolbar) { this.saveContent(editorElement); } + this.inputFocus.emit(false); }; editorElement.addEventListener('blur', this.blurListener); + +editorElement.addEventListener('focus', () => { + this.inputFocus.emit(true); +});Alternatively, update all parent components to remove the
(inputFocus)binding if focus tracking is no longer needed.
🧹 Nitpick comments (1)
editor/src/app/inputs/rich-text-input.component.ts (1)
120-129: Clarify the toolbar reset logic.The comment on line 125 says "Don't reset if already reset," but the check
if (!this.isClickingToolbar) return;prevents resetting when the flag is already false. This check is redundant since resetting a false flag to false is harmless. The mouseup listener is attached todocument, so it fires even when not clicking the toolbar, making this path common.Consider simplifying or clarifying:
this.mouseUpListener = () => { if (this.timeoutRef) { window.clearTimeout(this.timeoutRef); } this.timeoutRef = window.setTimeout(() => { - if (!this.isClickingToolbar) return; // Don't reset if already reset this.isClickingToolbar = false; this.timeoutRef = null; }, 100); };Or if you want to preserve the optimization, clarify the comment:
- if (!this.isClickingToolbar) return; // Don't reset if already reset + if (!this.isClickingToolbar) return; // Skip if not interacting with toolbar
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
editor/src/app/inputs/rich-text-input.component.ts(3 hunks)editor/src/app/sites/media/entry-gallery-editor.component.ts(6 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
editor/src/app/sites/media/entry-gallery-editor.component.ts (1)
editor/src/app/sites/sections/entries/entries-state/section-entries-state.model.ts (1)
SectionEntryGalleryFile(22-32)
editor/src/app/inputs/rich-text-input.component.ts (1)
editor/src/app/sites/shared/setting.component.ts (1)
Component(9-228)
editor/src/app/sites/sections/background-gallery-editor.component.ts (1)
editor/src/app/sites/sections/sections-state/site-sections-state.model.ts (1)
SiteSectionBackgroundFile(39-47)
🪛 ast-grep (0.39.6)
editor/src/app/inputs/rich-text-input.component.ts
[warning] 108-108: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.lastSavedValue = editorElement.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (5)
editor/src/app/inputs/rich-text-input.component.ts (5)
1-14: LGTM! Well-structured lifecycle implementation.The addition of AfterViewInit and OnDestroy lifecycle hooks is appropriate for managing editor initialization and cleanup. The ViewChild import is necessary for accessing the editor component instance.
Also applies to: 33-35
86-92: LGTM! Clean state management.The ViewChild reference and private state variables are well-structured for managing editor lifecycle and toolbar interactions.
94-100: LGTM! Efficient change detection.The
saveContentmethod correctly prevents duplicate emissions by comparing withlastSavedValue. ReadinginnerHTMLis safe here since the content originates from the angular-editor component, which handles sanitization.Note: The static analysis warning about innerHTML at line 108 is a false positive—the code reads from innerHTML to detect changes, it doesn't write unsanitized user input to it.
133-138: LGTM! Appropriate blur handling.The blur listener correctly prevents saving during toolbar interactions by checking
isClickingToolbar. This ensures content is only saved when the user truly leaves the editor, not when clicking toolbar buttons.
141-176: LGTM! Thorough cleanup implementation.The
ngOnDestroymethod properly cleans up all event listeners and timeouts in the correct order. The finalsaveContentcall (line 174) is essential to preserve any unsaved changes when the component is destroyed, even though emitting during destruction is generally discouraged—in this case, it's necessary to prevent data loss.
| ngAfterViewInit() { | ||
| // Get the contenteditable div element | ||
| const editorElement = this.editor.textArea.nativeElement; | ||
| this.lastSavedValue = editorElement.innerHTML; // Store initial value |
There was a problem hiding this comment.
Add null safety check for ViewChild.
The editor ViewChild is accessed without a null check. If the ViewChild query fails to resolve (e.g., due to template changes or delayed rendering), this will cause a runtime error.
Apply this diff to add null safety:
ngAfterViewInit() {
+ if (!this.editor?.textArea) {
+ console.error('RichTextInput: editor ViewChild not resolved');
+ return;
+ }
+
// Get the contenteditable div element
const editorElement = this.editor.textArea.nativeElement;
this.lastSavedValue = editorElement.innerHTML; // Store initial value🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 108-108: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.lastSavedValue = editorElement.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
In editor/src/app/inputs/rich-text-input.component.ts around lines 106 to 109,
the ngAfterViewInit reads this.editor.textArea.nativeElement without null
checks; add a null-safety guard that verifies this.editor and
this.editor.textArea (and nativeElement) are defined before accessing innerHTML,
and if any are missing either return early (no-op) or set lastSavedValue to an
empty string / appropriate default, and optionally log a warning so the
component won’t throw at runtime when the ViewChild isn't resolved.
| // Debounce file selection to allow pending updates to complete first | ||
| this.fileSelectionSubject$ | ||
| .pipe( | ||
| debounceTime(200), // Wait 200ms for any pending updates | ||
| takeUntilDestroyed(this.destroyRef), | ||
| ) | ||
| .subscribe((file) => { | ||
| this.setSelectedFile(file); | ||
| }); |
There was a problem hiding this comment.
Debouncing file selection is still lossy
The fixed 200 ms delay doesn’t guarantee the caption/autoplay save has finished before we switch files. In real deployments, debounced autosaves that lack an explicit “save complete” signal still drop edits or resurrect stale state when latency spikes or acknowledgements arrive late.(ehosseini.info)
Please gate selection on the actual save lifecycle instead of elapsed time—e.g., track an in-flight save flag or await the store/effect acknowledgement before allowing setSelectedFile to run, so we never move off a file while its update is pending.(ehosseini.info)
| this.fileSelectionSubject$ | ||
| .pipe( | ||
| debounceTime(200), // Wait 200ms for any pending updates | ||
| takeUntilDestroyed(this.destroyRef), | ||
| ) | ||
| .subscribe((file) => { | ||
| this.setSelectedFile(file); | ||
| }); |
There was a problem hiding this comment.
Same autosave race here
This component repeats the 200 ms debounce pattern, so background items can also switch before their edits are durably saved. The same real-world auto-save failures apply: timed delays don’t prevent stale writes or dropped changes when the backend responds after the window.(ehosseini.info)
Please reuse whatever “wait for save completion” guard you add in the entry gallery (e.g., queue clicks until the last save resolves or observe a store flag) so the background editor never abandons a file mid-save.
🤖 Prompt for AI Agents
In editor/src/app/sites/sections/background-gallery-editor.component.ts around
lines 357-364, the 200ms debounce allows background items to switch before an
in-progress auto-save completes, causing stale/dropped edits; replace the
debounce-based gating with the same "wait for save completion" guard used in the
entry gallery (e.g., observe the store's saving flag or a Promise that resolves
when the last save finishes), and queue or block setSelectedFile calls until
that save has resolved; ensure subscriptions check the guard before calling
setSelectedFile and that queued file selections are applied immediately after
the save completes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
editor/src/app/inputs/rich-text-input.component.ts (1)
107-110: Critical: Add null safety check for ViewChild (still unaddressed).This issue was flagged in a previous review but remains unaddressed. Line 109 accesses
this.editor.textArea.nativeElementwithout null checks. If the ViewChild query fails to resolve (e.g., due to template changes, conditional rendering, or timing issues), this will throw a runtime error.Note that
ngOnDestroy(line 163) correctly uses optional chaining:this.editor?.textArea?.nativeElement. Apply the same pattern here.Apply this diff to add null safety:
ngAfterViewInit() { + if (!this.editor?.textArea?.nativeElement) { + console.error('RichTextInput: editor ViewChild not resolved'); + return; + } + // Get the contenteditable div element const editorElement = this.editor.textArea.nativeElement;
🧹 Nitpick comments (1)
editor/src/app/inputs/rich-text-input.component.ts (1)
121-131: Consider clarifying the toolbar interaction logic.The mouseup timeout logic works correctly but is somewhat complex:
- Document-level mouseup fires for any click (not just toolbar)
- After 100ms, the flag resets only if it was previously set
- The 100ms delay ensures blur processing completes before flag reset
While functional, this could benefit from clearer inline comments or a diagram explaining the interaction flow between mousedown → blur → mouseup → timeout.
Example clarification:
this.mouseUpListener = () => { + // Clear any pending timeout to prevent stale flag resets if (this.timeoutRef) { window.clearTimeout(this.timeoutRef); } + // Wait 100ms after mouseup to reset toolbar flag + // (ensures blur event has finished processing first) this.timeoutRef = window.setTimeout(() => { - if (!this.isClickingToolbar) return; // Skip if not interacting with toolbar + if (!this.isClickingToolbar) return; // Already false, nothing to reset this.isClickingToolbar = false; this.timeoutRef = null; }, 100); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/src/app/inputs/rich-text-input.component.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
editor/src/app/inputs/rich-text-input.component.ts (1)
editor/src/app/sites/shared/setting.component.ts (1)
Component(9-228)
🪛 ast-grep (0.39.6)
editor/src/app/inputs/rich-text-input.component.ts
[warning] 109-109: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.lastSavedValue = editorElement.innerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (6)
editor/src/app/inputs/rich-text-input.component.ts (6)
1-14: LGTM! Imports are correctly updated.The new imports for
ViewChild,AfterViewInit,OnDestroy, andAngularEditorComponentare all necessary for the lifecycle-based save flow.
20-20: LGTM! Template reference variable added correctly.The
#editorreference enables ViewChild access while preserving the existing ngModel binding.
33-36: LGTM! Lifecycle interfaces correctly implemented.The component properly extends its lifecycle hooks to support the new save-on-blur behavior.
87-93: LGTM! State management properties are well-defined.The ViewChild reference and state tracking properties are appropriately scoped and initialized for managing the save-on-blur behavior.
95-101: LGTM! Save logic correctly guards against redundant updates.The method properly compares content before emitting updates. Note: The static analysis XSS warning on line 96 is a false positive—the code reads
innerHTMLfrom the Angular Editor component (which handles sanitization via ngModel), it doesn't write unsanitized user input.
147-182: LGTM! Cleanup logic is thorough and well-ordered.The destroy sequence correctly:
- Clears pending timeouts to prevent delayed actions
- Resets toolbar state
- Removes all event listeners (preventing new events during teardown)
- Saves final content as the last step
- Uses optional chaining to safely access the editor reference
This prevents race conditions during component destruction and ensures unsaved changes are captured.
1 participant
Save value on blur, add a delay to allow finish saving before selecting other gallery items in gallery editor and background gallery editor.
Summary by CodeRabbit
New Features
Improvements