-
Notifications
You must be signed in to change notification settings - Fork 799
WEB-310-feat(editor): replace CKEditor5 with TinyMCE #2804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Removed unnecessary as unknown as EditorOptions type cast WEB-422 unit test coverage for ViewTaxComponentComponent feat(editor): implement self-hosted TinyMCE GPL editor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Editor Framework Migrationpackage.json, angular.json, src/typings.d.ts |
Replaced CKEditor 5 dependencies (@ckeditor/ckeditor5-angular, @ckeditor/ckeditor5-build-classic) with TinyMCE (@tinymce/tinymce-angular, tinymce); removed CKEditor from Angular build configuration allowedCommonJsDependencies; removed CKEditor type declarations. |
Template Component Editor Integrationsrc/app/templates/create-edit-template/create-edit-template.component.ts, src/app/templates/create-edit-template/create-edit-template.component.html, src/app/templates/create-edit-template/create-edit-template.component.scss |
Replaced CKEditor with TinyMCE EditorComponent; updated editor references, content insertion, and state management; added TinyMCE configuration (tinyMceInit) with plugins and toolbar; added onEditorBlur handler for touch tracking; introduced textFormControl getter for template binding; updated styling selectors and container dimensions. |
TinyMCE Configuration & Module Updatessrc/app/templates/tinymce-loader.ts, src/app/templates/templates.module.ts |
Added new tinymce-loader module configuring TinyMCE core, plugins (advlist, autolink, lists, link, charmap, preview, searchreplace, visualblocks, code, fullscreen, table, wordcount, image, media), and skins; updated NgModule to import EditorModule instead of CKEditorModule. |
New Test Coveragesrc/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts |
Added comprehensive integration test suite for ViewTaxComponentComponent with mocked Router, ActivatedRoute, AuthenticationService, and SettingsService; validates component lifecycle, route data subscription, rendering, edge cases, and sequential data updates. |
UI Component Cleanupsrc/app/configuration-wizard/configuration-wizard.component.ts |
Removed MatGridList and MatGridTile imports; removed unused public property show: number. |
Sequence Diagram
sequenceDiagram
participant Template as CreateEditTemplate<br/>Component
participant Editor as TinyMCE<br/>EditorComponent
participant Form as Reactive Form<br/>(textFormControl)
participant Module as tinymce-loader<br/>(Configuration)
Template->>Module: Import tinyMceInit config
Module-->>Template: Configure plugins, toolbar, skins
Template->>Editor: Bind [init]="tinyMceInit"
Template->>Editor: Bind [formControl]="textFormControl"
alt User Edits Content
Editor->>Template: Content change
Template->>Form: Sync form control value
end
alt User Blurs Editor
Editor->>Template: (onBlur) event
Template->>Form: Mark control as touched
end
Template->>Template: onTemplateLoad/entityChanges
Template->>Editor: Clear content
Template->>Form: Reset textControl (pristine/untouched)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Editor integration: The CKEditor-to-TinyMCE migration spans multiple component files with interdependent changes in template binding, styling, and logic; verify form control synchronization, content insertion via
addText(), and state reset behavior during template load/entity changes. - TinyMCE configuration: Verify tinymce-loader module properly exposes TinyMCE globally, plugin/toolbar selections are appropriate, and skins/themes load correctly.
- Test file: Review mock setup completeness (ActivatedRoute BehaviorSubject, AuthenticationService, SettingsService), edge case coverage (null/undefined fields, nested objects, arrays), and performance test validity.
- Grid component removal: Confirm
show: numberproperty is no longer referenced elsewhere in the codebase.
Possibly related PRs
- WEB-424 "MatGridList is not used" and "MatGridTile is not used" warnings during startup of "ng serve" #2801 — Modifies the same configuration-wizard.component.ts file to remove MatGridList/MatGridTile (overlapping UI cleanup).
- WEB-422 unit test coverage for ViewTaxComponentComponent #2802 — Adds the identical view-tax-component.component.spec.ts test file (duplicate test coverage).
Suggested reviewers
- alberto-art3ch
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title accurately describes the main change: replacing CKEditor5 with TinyMCE. It is concise, specific, and directly reflects the primary objective of the changeset across all modified files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts (1)
1-560: Unrelated test file in TinyMCE migration PR.This comprehensive test suite for
ViewTaxComponentComponentappears unrelated to the stated PR objective of replacing CKEditor with TinyMCE in the template editor. Consider moving this to a separate PR focused on tax component testing.Additionally, some test cases are overly granular:
- Lines 194-204: Testing that a string/number has the correct type is unnecessary
- Lines 527-559: Performance tests with 100 iterations belong in dedicated performance/load test suites rather than unit tests
The PR would benefit more from integration tests covering the TinyMCE editor functionality in the template components.
src/app/templates/tinymce-loader.ts (1)
6-20: Verify all plugins are necessary.The loader imports 14 TinyMCE plugins. Consider whether all plugins are actively used to minimize bundle size and potential attack surface:
advlist,autolink,lists(list functionality)link,image,media(embedding content)charmap,table,blockquote(formatting)preview,code,fullscreen(utilities)searchreplace,visualblocks,wordcount(editing aids)Review the actual toolbar configuration in
create-edit-template.component.ts(lines 72-74) and remove unused plugins to reduce bundle size.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (10)
angular.json(0 hunks)package.json(2 hunks)src/app/configuration-wizard/configuration-wizard.component.ts(0 hunks)src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts(1 hunks)src/app/templates/create-edit-template/create-edit-template.component.html(1 hunks)src/app/templates/create-edit-template/create-edit-template.component.scss(2 hunks)src/app/templates/create-edit-template/create-edit-template.component.ts(5 hunks)src/app/templates/templates.module.ts(2 hunks)src/app/templates/tinymce-loader.ts(1 hunks)src/typings.d.ts(0 hunks)
💤 Files with no reviewable changes (3)
- src/typings.d.ts
- src/app/configuration-wizard/configuration-wizard.component.ts
- angular.json
🧰 Additional context used
📓 Path-based instructions (2)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/templates/tinymce-loader.tssrc/app/templates/create-edit-template/create-edit-template.component.scsssrc/app/templates/templates.module.tssrc/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.tssrc/app/templates/create-edit-template/create-edit-template.component.tssrc/app/templates/create-edit-template/create-edit-template.component.html
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: For tests: encourage clear Arrange-Act-Assert structure,
stable selectors, and minimal brittle timing dependencies.
Files:
src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts
🧬 Code graph analysis (1)
src/app/templates/templates.module.ts (1)
src/app/app.module.ts (1)
NgModule(79-136)
🔇 Additional comments (7)
src/app/templates/templates.module.ts (1)
5-5: LGTM! Clean module migration to TinyMCE.The module correctly replaces
CKEditorModulewithEditorModulefrom@tinymce/tinymce-angular, and the comment clarifies the local bundling strategy viatinymce-loader.ts.Also applies to: 19-19, 23-23
src/app/templates/create-edit-template/create-edit-template.component.scss (1)
17-17: LGTM! Clean styling migration to TinyMCE.The SCSS correctly updates selectors from CKEditor (
.ck-editor,.ck-content) to TinyMCE (.tox-tinymce,.tox-edit-area__iframe), adds appropriate error styling, and adjusts the minimum height to accommodate TinyMCE's layout.Also applies to: 32-48
src/app/templates/create-edit-template/create-edit-template.component.html (1)
84-97: LGTM! Clean template migration with improved accessibility.The template correctly replaces CKEditor with TinyMCE's
<editor>component, properly binds the form control, and includes appropriate validation error display. The addition of theidattribute and correspondingforattribute on the label improves accessibility.src/app/templates/create-edit-template/create-edit-template.component.ts (3)
234-249: LGTM! Robust TinyMCE integration with proper fallbacks.The
addText,getEditorContent,setEditorContent, andonEditorBlurmethods handle TinyMCE integration correctly with appropriate fallbacks when the editor isn't available. The form control synchronization and validation triggering are well-implemented.Also applies to: 254-274
97-103: Good abstraction with getter methods.The
textControl(private) andtextFormControl(public) getters provide clean access to the form control for internal logic and template binding respectively. This separation follows Angular best practices.
46-77: Configuration is correct; review comment is inaccurate.The TinyMCE setup properly bundles skins via
tinymce-loader.tsimports and correctly disables external loading. Settingskin: falseandcontent_css: []when skins are imported as modules (not loaded from URLs) is the correct pattern. The component's comment explicitly states "using bundled version via tinymce-loader.ts," andbase_url: ''/suffix: ''further confirm this is intentional. No configuration changes are needed.Likely an incorrect or invalid review comment.
package.json (1)
55-55: TinyMCE packages are current and secure—no vulnerabilities detected in specified versions.The script confirms that both
tinymce@8.2.2and@tinymce/tinymce-angular@9.1.1are the latest versions available. While several MODERATE XSS vulnerabilities exist for tinymce in earlier versions (< 5.11.0, 6.0.0–6.8.4, 7.0.0–7.2.0), version 8.2.2 is outside all vulnerable ranges. No security advisories are present for @tinymce/tinymce-angular. The codebase is secure with these versions.
| plugins: [ | ||
| 'advlist', | ||
| 'autolink', | ||
| 'lists', | ||
| 'link', | ||
| 'charmap', | ||
| 'preview', | ||
| 'searchreplace', | ||
| 'visualblocks', | ||
| 'code', | ||
| 'fullscreen', | ||
| 'table', | ||
| 'wordcount', | ||
| 'image', | ||
| 'media' | ||
| ], | ||
| toolbar: | ||
| 'undo redo | blocks | bold italic | link image table blockquote media | ' + | ||
| 'bullist numlist | outdent indent | alignleft aligncenter alignright alignjustify | removeformat code fullscreen preview', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add security controls for media and image features.
The toolbar includes image, media, table, and link features without security restrictions. For user-generated content, consider adding:
- Image upload control: Set
images_upload_handlerto validate/sanitize uploads orimages_upload_urlto route through a secure endpoint - Media URL validation: Set
media_url_resolverto validate and sanitize media URLs - Link target restrictions: Consider
target_listorlink_assume_external_targetsto control link behavior - Disable uploads entirely if not needed: Remove
imagefrom plugins/toolbar if users shouldn't upload images
Example security configuration:
readonly tinyMceInit: Partial<EditorOptions> = {
// ... existing config
// Option 1: Disable image uploads entirely
images_upload_handler: (blobInfo, progress) => {
return Promise.reject('Image uploads are disabled');
},
// Option 2: Route through secure backend
images_upload_url: '/api/templates/upload-image',
images_upload_credentials: true,
// Validate media URLs
media_url_resolver: (data, resolve, reject) => {
// Validate against allowlist or sanitize URL
if (isAllowedMediaUrl(data.url)) {
resolve({ html: data.html });
} else {
reject('Invalid media URL');
}
},
// Control link targets
link_assume_external_targets: 'https',
target_list: false // or provide specific targets
};🤖 Prompt for AI Agents
In src/app/templates/create-edit-template/create-edit-template.component.ts
around lines 56-74 the TinyMCE config exposes image, media, table and link
features without any upload/URL validation or target restrictions; to fix, add
either an images_upload_handler that validates/sanitizes blobs (or reject
uploads) or set images_upload_url to a secure backend endpoint with credentials,
implement media_url_resolver to validate/sanitize incoming media URLs and reject
disallowed hosts, and restrict link behavior via link_assume_external_targets or
target_list (or remove the image/media plugins and toolbar buttons entirely if
uploads/embeds should be disabled); ensure these handlers return/reject Promises
per TinyMCE API and log/propagate errors to the backend for auditing.
| import 'tinymce/plugins/image'; | ||
| import 'tinymce/plugins/media'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify security configuration for media plugins.
The image and media plugins can introduce security risks if users can embed arbitrary external content. Ensure that the TinyMCE configuration in create-edit-template.component.ts includes appropriate restrictions:
images_upload_handlerorimages_upload_urlto control image uploadsmedia_url_resolverto validate/sanitize media URLs- Content Security Policy (CSP) headers to restrict external resources
- Input sanitization for user-generated content
Check the TinyMCE initialization config for security settings:
🏁 Script executed:
#!/bin/bash
# Description: Verify TinyMCE security configurations
echo "=== Checking for TinyMCE security configurations ==="
rg -nP -A5 -B5 'tinyMceInit|images_upload|media_url_resolver|file_picker' --type=tsLength of output: 1272
I'll retrieve the full TinyMCE configuration to verify security settings.
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Get full tinyMceInit configuration
echo "=== Full tinyMceInit configuration ==="
rg -nP -A50 'readonly tinyMceInit' --type=ts src/app/templates/create-edit-template/
Length of output: 5416
🏁 Script executed:
#!/bin/bash
# Description: Check for image/media security configurations
echo "=== Checking for security-related configurations ==="
rg -nP 'images_upload_handler|images_upload_url|media_url_resolver|file_picker_callback|plugins:.*image|plugins:.*media' --type=ts src/app/templates/Length of output: 52
🏁 Script executed:
#!/bin/bash
# Description: Check for Content-Security-Policy headers and sanitization
echo "=== Checking for CSP and sanitization ==="
rg -nP 'Content-Security-Policy|sanitize|DomSanitizer|bypassSecurityTrustHtml' --type=ts src/app/templates/Length of output: 41
🏁 Script executed:
#!/bin/bash
# Description: Check tinymce-loader for plugin configurations
echo "=== Checking tinymce-loader.ts for any configurations ==="
cat src/app/templates/tinymce-loader.tsLength of output: 940
🏁 Script executed:
#!/bin/bash
# Description: Check application-level CSP configuration
echo "=== Checking for CSP in HTML/Angular config ==="
find . -type f \( -name "index.html" -o -name "angular.json" -o -name "*.config.ts" \) -exec grep -l "Content-Security-Policy" {} \;Length of output: 47
I need to verify if sanitization is applied elsewhere in the component or globally in the application.
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Search for sanitization across entire codebase
echo "=== Checking for DomSanitizer and sanitization globally ==="
rg -nP 'DomSanitizer|sanitize|bypassSecurityTrustHtml|bypassSecurityTrustResourceUrl' --type=ts
Length of output: 6634
🏁 Script executed:
#!/bin/bash
# Description: Check create-edit-template component for content handling
echo "=== Checking content save/load handlers in create-edit-template ==="
rg -nP -A10 'onSave|saveTemplate|updateTemplate|ngOnInit|getValue|getContent' --type=ts src/app/templates/create-edit-template/create-edit-template.component.ts | head -100Length of output: 1544
🏁 Script executed:
#!/bin/bash
# Description: Check for any HTML sanitization or validation utilities
echo "=== Checking for sanitization utilities in the project ==="
find . -type f -name "*.ts" -path "*/src/*" ! -path "*/node_modules/*" | xargs grep -l "sanitize\|DomSanitizer" 2>/dev/null | head -20Length of output: 1106
🏁 Script executed:
#!/bin/bash
# Description: Check index.html for CSP meta tags
echo "=== Checking index.html for CSP meta tags ==="
grep -i "csp\|content-security-policy" index.html 2>/dev/null || echo "No CSP found in index.html"Length of output: 72
I need to check where template content is rendered to determine if sanitization occurs downstream.
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Search for where template content is used/rendered
echo "=== Checking where template content is displayed ==="
rg -nP 'getTemplate|templateContent|displayTemplate|innerHTML|template\.text|template\.content' --type=ts src/app/templates/ | grep -v 'create-edit-template' | head -50
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Description: Check template service for sanitization
echo "=== Checking template service for any sanitization ==="
fd -e ts template.service | xargs cat | head -100Length of output: 54
🏁 Script executed:
#!/bin/bash
# Description: Check if DomSanitizer is injected in create-edit-template
echo "=== Checking imports and constructor in create-edit-template ==="
rg -nP -A15 'export class CreateEditComponent' --type=ts src/app/templates/create-edit-template/create-edit-template.component.tsLength of output: 621
Configure TinyMCE to restrict image and media uploads and validate embedded content.
Verification confirms the image and media plugins are enabled without security controls. The configuration lacks:
images_upload_handlerto restrict uploads to server-only or disabledmedia_url_resolverto validate/sanitize embedded media URLs- Content sanitization when persisting via
getContent()at line 256 increate-edit-template.component.ts - Application-level CSP headers
Required actions:
- Add
images_upload_handlerto disable uploads or validate against a whitelist - Add
media_url_resolverto validate media URLs or disallow external media - Inject
DomSanitizerand sanitize content before saving increate-edit-template.component.ts(line 300updateTemplate()call) - Consider adding CSP headers in
index.htmlor application configuration to restrict external resource loading
alberto-art3ch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments, and try to review the coderrabit suggestions
| @@ -26,4 +25,4 @@ | |||
| CdkScrollable, | |||
| MatDialogContent, | |||
| MatDialogClose, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase with the dev branch because this change was already done in other PR
Replaced CKEditor with TinyMCE in the template editor
Dependencies
@tinymce/tinymce-angular: ^9.1.1 - Angular wrapper for TinyMCE
tinymce: ^8.2.2` - TinyMCE core library
#{Issue Number}
WEB-310
Screen.Recording.2025-11-20.192705.mp4
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Tests
Refactor
Chores