[7418] RTE control#4822
Open
jvega190 wants to merge 19 commits intocraftercms:developfrom
Open
Conversation
Contributor
WalkthroughAdds a ChangesRich Text Editor & TinyMCE Initialization
Sequence DiagramsequenceDiagram
participant Descriptor as RTE Descriptor
participant FormsEngine as FormsEngine RTE
participant Utilities as formUtils.getTinyMceInitOptions
participant Guest as Guest initTinyMCE
participant TinyMCE as TinyMCE
Descriptor->>FormsEngine: field includes `maxlength`
FormsEngine->>Utilities: request init options (field, rteConfig, defaults, setup)
Utilities->>Utilities: assemble init (plugins, skins/CSS, paste/file hooks, maxLength)
Utilities-->>FormsEngine: return init options
FormsEngine->>TinyMCE: initialize editor with returned options
TinyMCE-->>FormsEngine: content events / ready
Descriptor->>Guest: field includes `maxlength`
Guest->>Utilities: request init options (field, rteSetup.tinymceOptions, {}, setup)
Utilities-->>Guest: return rteConfig (includes paste_preprocess enforcing maxLength)
Guest->>TinyMCE: window.tinymce.init(rteConfig)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
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 (2)
ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.ts (1)
29-61:⚠️ Potential issue | 🟠 Major
maxLengthis being stored in the wrong bucket for the existing consumers.The current validation plumbing reads
field.validations.maxLength(validators.ts, the guest RTE max-length guards, and the existing Numeric control pattern). Adding the new setting under the"properties"section means the configured limit will not reach those paths. Either model this as a constraint/validation or update every consumer to readfield.properties.maxLengthconsistently.🧭 One likely fix
createVirtualSection({ id: 'properties', title: defineMessage({ defaultMessage: 'Options' }), fields: [ 'height', - 'maxLength', 'autoGrow', 'enableSpellCheck', 'rteConfiguration', 'imageManager', 'videoManager', @@ createVirtualSection({ id: 'constraints', title: defineMessage({ defaultMessage: 'Constraints' }), - fields: ['required'] + fields: ['required', 'maxLength'] })Based on learnings, Numeric controls read
maxLengthfromfield.validations?.maxLength?.value, so storing the RTE value underpropertiesdiverges from the existing FormsEngine pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.ts` around lines 29 - 61, The maxLength setting for the RTE is placed under the properties bucket but consumers (e.g., validators.ts and the guest RTE max-length guards and the Numeric control pattern) expect field.validations.maxLength.value; update the RTE descriptor so the maxLength entry is stored under the field.validations shape (matching how Numeric control reads field.validations?.maxLength?.value) or alternatively update all consumers to read field.properties.maxLength (prefer the former to keep consistency) — modify the maxLength definition in this file (the maxLength field descriptor) to populate validations.maxLength with the appropriate structure and default value.ui/guest/src/controls/rte.ts (1)
165-408:⚠️ Potential issue | 🟠 MajorThis callback still carries pre-helper behavior.
getTinyMceInitOptions()already wiresDblClick,templates_css, andcraftercms_tinymce_hooks. Keeping the same registrations here doubles those side effects, and the oldif (type !== 'html')guard now also disablesmeta+b/i/ufortype === 'rte'. Keep only guest-specific logic in this callback and switch the plain-text check to!isRTE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/guest/src/controls/rte.ts` around lines 165 - 408, The callback duplicates work already done in getTinyMceInitOptions by re-registering DblClick, templates_css and craftercms_tinymce_hooks and incorrectly uses the plain-text check (type !== 'html') to disable formatting shortcuts; remove the duplicated registrations (the editor.on('DblClick', ...), the editor.options.register/set('templates_css', ...), and the pluginManager.waitFor('craftercms_tinymce_hooks', ...) block) and change the guard that removes meta+b/meta+i/meta+u from if (type !== 'html') to if (!isRTE) so shortcuts are only disabled for plain-text (non-RTE) fields; leave only guest-specific logic in this callback and rely on getTinyMceInitOptions for shared TinyMCE wiring.
🧹 Nitpick comments (1)
ui/guest/rollup.config.mjs (1)
24-24: Missing semicolon for consistency.All other import statements in this file end with semicolons. Consider adding one here for consistency.
🔧 Suggested fix
-import svg from 'rollup-plugin-svg' +import svg from 'rollup-plugin-svg';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/guest/rollup.config.mjs` at line 24, The import statement "import svg from 'rollup-plugin-svg'" is missing a trailing semicolon; update that import to include a semicolon so it matches the styling of other imports in the file (i.e., change the "import svg" line to end with a semicolon).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/RichTextEditor.tsx`:
- Around line 184-187: The RTE currently only shows maxLength in the
FormsEngineField counter but does not prevent or validate input beyond that
limit; update getTinyMceInitOptions to enforce the configured maxLength for the
rich text control (use TinyMCE init handlers such as setup with
input/keyDown/keypress and paste handlers to block or truncate content to
maxLength) and also add a validator path in validators.ts for 'rte' that
enforces the same maxLength server/submit-side; ensure handleChange still calls
the commit but that the content is truncated/blocked before commit so users
cannot type or paste past the configured limit.
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 949-952: The mapping for spell checking is inverted: fix the logic
in formUtils.tsx where controlProps.browser_spellcheck is set based on
field.properties?.enableSpellCheck?.value so that spellcheck is enabled by
default and only disabled when enableSpellCheck.value === false; specifically
replace the current conditional that sets browser_spellcheck to true on false
with an explicit assignment like controlProps.browser_spellcheck =
field.properties?.enableSpellCheck?.value !== false (or equivalent) so the
property is true when undefined/true and false only when explicitly false,
affecting the shared helper used by FormsEngine and guest RTEs.
In `@ui/guest/src/controls/rte.ts`:
- Around line 129-163: The code currently spreads rteSetup.tinymceOptions then
unconditionally sets toolbar: isRTE and menubar: false, which overwrites any
site-specific toolbar/menubar config; change the tinymceOptions construction so
it preserves any toolbar or menubar provided by rteSetup.tinymceOptions (e.g.
use toolbar: rteSetup.tinymceOptions.toolbar ?? isRTE and menubar:
rteSetup.tinymceOptions.menubar ?? false or spread defaults first then spread
rteSetup.tinymceOptions and finally apply only the runtime fields like target,
deprecation_warnings, paste_as_text, paste_data_images, inline,
code_editor_inline and paste_preprocess), updating the tinymceOptions object
inside getTinyMceInitOptions (the rteSetup.tinymceOptions block referenced in
rteConfig) so site-configured toolbar/menubar are not lost.
---
Outside diff comments:
In `@ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.ts`:
- Around line 29-61: The maxLength setting for the RTE is placed under the
properties bucket but consumers (e.g., validators.ts and the guest RTE
max-length guards and the Numeric control pattern) expect
field.validations.maxLength.value; update the RTE descriptor so the maxLength
entry is stored under the field.validations shape (matching how Numeric control
reads field.validations?.maxLength?.value) or alternatively update all consumers
to read field.properties.maxLength (prefer the former to keep consistency) —
modify the maxLength definition in this file (the maxLength field descriptor) to
populate validations.maxLength with the appropriate structure and default value.
In `@ui/guest/src/controls/rte.ts`:
- Around line 165-408: The callback duplicates work already done in
getTinyMceInitOptions by re-registering DblClick, templates_css and
craftercms_tinymce_hooks and incorrectly uses the plain-text check (type !==
'html') to disable formatting shortcuts; remove the duplicated registrations
(the editor.on('DblClick', ...), the
editor.options.register/set('templates_css', ...), and the
pluginManager.waitFor('craftercms_tinymce_hooks', ...) block) and change the
guard that removes meta+b/meta+i/meta+u from if (type !== 'html') to if (!isRTE)
so shortcuts are only disabled for plain-text (non-RTE) fields; leave only
guest-specific logic in this callback and rely on getTinyMceInitOptions for
shared TinyMCE wiring.
---
Nitpick comments:
In `@ui/guest/rollup.config.mjs`:
- Line 24: The import statement "import svg from 'rollup-plugin-svg'" is missing
a trailing semicolon; update that import to include a semicolon so it matches
the styling of other imports in the file (i.e., change the "import svg" line to
end with a semicolon).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c980a64b-6f10-431b-b3c3-0944419c083d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.tsui/app/src/components/FormsEngine/controls/RichTextEditor.tsxui/app/src/components/FormsEngine/lib/formUtils.tsxui/guest/package.jsonui/guest/rollup.config.mjsui/guest/src/controls/rte.ts
added 3 commits
April 1, 2026 10:13
…o feature/7418-richTextEditor # Conflicts: # ui/app/src/components/FormsEngine/controls/RichTextEditor.tsx # ui/app/src/components/FormsEngine/lib/formUtils.tsx # ui/guest/src/controls/rte.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ui/guest/src/controls/rte.ts (1)
404-406: MovemaxLengthdefinition before its usage for clarity.
maxLengthis defined at line 404 but referenced inside thepaste_preprocesscallback (line 138). While this works due to JavaScript closure semantics (the callback executes later whenmaxLengthis already defined), the variable ordering is confusing and error-prone during refactoring.♻️ Suggested refactor
Move the
maxLengthdefinition before the config object:record.element.classList.remove(emptyFieldClass); + const maxLength = validations?.maxLength ? parseInt(validations.maxLength.value) : null; + const rteConfig = getTinyMceInitOptions( field, { // ... config object } ); - const maxLength = validations?.maxLength ? parseInt(validations.maxLength.value) : null; // `@ts-expect-error` - Typings state the prop is wrong for the React integration, but the prop is correct. window.tinymce.init(rteConfig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/guest/src/controls/rte.ts` around lines 404 - 406, Move the maxLength definition above the rteConfig so it is declared before any references inside the paste_preprocess callback: locate the maxLength variable and the rteConfig object (used by window.tinymce.init) and place the const maxLength = validations?.maxLength ? parseInt(validations.maxLength.value) : null; immediately before constructing rteConfig so paste_preprocess (and any other callbacks on rteConfig) reference a clearly defined variable.ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
1000-1006: Consider extracting dark mode detection to avoid repetition.
window.matchMedia('(prefers-color-scheme: dark)').matchesis called multiple times (lines 1000, 1004, and 1237). Extracting it to a variable would improve readability and ensure consistency.♻️ Suggested refactor
export function getTinyMceInitOptions( field: ContentTypeField, rteConfig: GlobalState['preview']['richTextEditor'], defaultOptions?: Editor['props']['init'], setup?: Editor['props']['init']['setup'] ): Editor['props']['init'] { const setupId: string = getPropertyValue(field.properties, 'rteConfiguration', 'generic') as string; const height = getPropertyValue(field.properties, 'height', 300) as number; const autoGrow = getPropertyValue(field.properties, 'autoGrow', false) as boolean; const allowAddMedia = getValidationValue(field.validations, 'addMedia', true) as boolean; + const isDarkMode = window.matchMedia('(prefers-color-scheme: dark)').matches; // ... - skin: window.matchMedia('(prefers-color-scheme: dark)').matches ? 'oxide-dark' : 'oxide', + skin: isDarkMode ? 'oxide-dark' : 'oxide', content_css: (tinymceOptions?.content_css as string | string[])?.length ? tinymceOptions.content_css - : window.matchMedia('(prefers-color-scheme: dark)').matches + : isDarkMode ? 'dark' : 'default',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx` around lines 1000 - 1006, Extract the repeated dark-mode check into a single boolean (e.g., prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches) at the top of the scope where TinyMCE options are built (the function that sets skin and content_css in formUtils), then replace all occurrences of window.matchMedia('(prefers-color-scheme: dark').matches (used for skin, content_css and the other occurrence around the TinyMCE config) with prefersDark so the logic is consistent and not duplicated; ensure the new variable is in the same local scope as references to tinymceOptions, skin, and content_css to avoid changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-512: The rteValidator currently assigns currentValue directly
to aux.innerHTML which converts null/undefined to the strings "null"/"undefined"
and yields a false-positive non-empty result; update the rteValidator function
to first guard against null/undefined (e.g., if currentValue == null) and return
false (or normalize currentValue to an empty string before assigning to
aux.innerHTML), then proceed to extract aux.innerText.trim() as now; reference
the rteValidator function and the currentValue variable when making the change.
In `@ui/guest/src/controls/rte.ts`:
- Around line 120-155: rteSetup.id can be undefined which produces a computed
property key of "undefined"—fix by adding null-safety and resolving the type
mismatch: either make the RteSetup.id required (update the RteSetup interface)
or guard the call site and this block so you never pass an empty rteSetup
without an id; in this file compute a safe key (e.g. const setupId = rteSetup.id
?? 'rte') and use setupId instead of rteSetup.id in the getTinyMceInitOptions
call (or only include the object when rteSetup.id is present) so the computed
property and types are correct and you avoid using "undefined" as a key; ensure
root.ts call site that passes {} is updated to provide an id when type === 'rte'
if you choose to require the id.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/formUtils.tsx`:
- Around line 1000-1006: Extract the repeated dark-mode check into a single
boolean (e.g., prefersDark = window.matchMedia('(prefers-color-scheme:
dark)').matches) at the top of the scope where TinyMCE options are built (the
function that sets skin and content_css in formUtils), then replace all
occurrences of window.matchMedia('(prefers-color-scheme: dark').matches (used
for skin, content_css and the other occurrence around the TinyMCE config) with
prefersDark so the logic is consistent and not duplicated; ensure the new
variable is in the same local scope as references to tinymceOptions, skin, and
content_css to avoid changing behavior.
In `@ui/guest/src/controls/rte.ts`:
- Around line 404-406: Move the maxLength definition above the rteConfig so it
is declared before any references inside the paste_preprocess callback: locate
the maxLength variable and the rteConfig object (used by window.tinymce.init)
and place the const maxLength = validations?.maxLength ?
parseInt(validations.maxLength.value) : null; immediately before constructing
rteConfig so paste_preprocess (and any other callbacks on rteConfig) reference a
clearly defined variable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e584ddb1-73de-4545-bbeb-88563451b539
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.tsui/app/src/components/FormsEngine/FormsEngineDialog.tsxui/app/src/components/FormsEngine/controls/RichTextEditor.tsxui/app/src/components/FormsEngine/lib/formUtils.tsxui/app/src/components/FormsEngine/lib/validators.tsui/app/src/components/FormsEngine/lib/valueSerializers.tsui/guest/src/controls/rte.ts
✅ Files skipped from review due to trivial changes (1)
- ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/src/components/FormsEngine/controls/RichTextEditor.tsx
4 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
503-516:⚠️ Potential issue | 🟠 MajorRTE validator incorrectly treats nullish/optional empty values as invalid.
Line 507 can coerce
null/undefinedinto text, and Line 510 enforces non-empty content even when the field is not required. This can produce false positives and invalid optional RTE states.🐛 Proposed fix
-const rteValidator = (field: ContentTypeField, currentValue: string, messages?: FieldValidityMessage[]): boolean => { +const rteValidator = ( + field: ContentTypeField, + currentValue: string | null | undefined, + messages: FieldValidityMessage[] = [] +): boolean => { if (nou(field)) return false; + const required = isFieldRequired(field); + if (nou(currentValue)) return !required; const aux = document.createElement('div'); aux.innerHTML = currentValue; const trimmedContent = aux.innerText.trim(); // Get only the text and remove white spaces - const isValid = trimmedContent !== ''; + const isValid = !required || trimmedContent !== ''; if (!isValid) { messages.push(defineMessage({ defaultMessage: 'This field is required.' })); } return isValid; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 503 - 516, The rteValidator currently treats nullish/optional values as invalid and blindly coerces currentValue into text; update rteValidator to early-return true (valid) when the field or the currentValue is null/undefined or the field is not required (use field.required), only run the DOM parsing/text-trimming when typeof currentValue === 'string' and field.required is true, and guard messages before pushing (e.g., if (messages) messages.push(...)); keep references to rteValidator, field.required, currentValue, messages, and defineMessage to locate the changes.ui/guest/src/controls/rte.ts (1)
132-133:⚠️ Potential issue | 🟠 MajorPreserve site-configured toolbar/menubar for RTE setups.
At Line 132 and Line 133,
toolbar: isRTEandmenubar: isRTEoverwrite any configured values fromrteSetup.tinymceOptions, so custom editor UI config is lost.Suggested patch
- toolbar: isRTE, - menubar: isRTE, + toolbar: isRTE ? (rteSetup?.tinymceOptions?.toolbar ?? true) : false, + menubar: isRTE ? (rteSetup?.tinymceOptions?.menubar ?? false) : false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/guest/src/controls/rte.ts` around lines 132 - 133, The current assignment `toolbar: isRTE` and `menubar: isRTE` overwrites any custom values in `rteSetup.tinymceOptions`; change the merge logic where TinyMCE options are composed so that `rteSetup.tinymceOptions.toolbar` and `.menubar` take precedence if present, and only fall back to `isRTE` when those keys are undefined or null. Update the code that builds the final options (look for `toolbar`, `menubar`, `isRTE`, and `rteSetup.tinymceOptions` in this file) to conditionally use the existing `rteSetup.tinymceOptions` values instead of unconditionally setting them to `isRTE`.
🧹 Nitpick comments (1)
ui/guest/src/controls/rte.ts (1)
124-124: Use a computed property for the setup map key.At Line 124,
setupId:creates a literal"setupId"key instead of using the resolved id value. This currently works only via fallback-to-first-entry behavior and is fragile.Suggested patch
- { - setupId: { + { + [setupId]: { id: setupId, tinymceOptions: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/guest/src/controls/rte.ts` at line 124, The object literal currently uses the literal key "setupId" instead of the resolved variable value, so replace the literal key with a computed property using the setupId variable (e.g., change setupId: to [setupId]:) wherever the setup map is built in rte.ts so the actual id is used as the map key (look for the map/object creation referencing setupId in the RTE setup code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/guest/src/controls/rte.ts`:
- Around line 137-152: The paste length calculation mixes formats
(currentContent is plain text while args.content is HTML), so normalize to a
single format before measuring and truncating: convert args.content from HTML to
plain text (strip tags/entities) and use that plain-text version for
fullContent, maxLengthExceeded checks, the snackGuestMessage values, and for
truncation; then set args.content to the truncated plain-text string (or to the
appropriately re-encoded HTML if you prefer HTML output) so that
editor.getContent({ format: 'text' }), args.content, maxLengthExceeded, and the
truncation all use the same text format.
---
Duplicate comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-516: The rteValidator currently treats nullish/optional values
as invalid and blindly coerces currentValue into text; update rteValidator to
early-return true (valid) when the field or the currentValue is null/undefined
or the field is not required (use field.required), only run the DOM
parsing/text-trimming when typeof currentValue === 'string' and field.required
is true, and guard messages before pushing (e.g., if (messages)
messages.push(...)); keep references to rteValidator, field.required,
currentValue, messages, and defineMessage to locate the changes.
In `@ui/guest/src/controls/rte.ts`:
- Around line 132-133: The current assignment `toolbar: isRTE` and `menubar:
isRTE` overwrites any custom values in `rteSetup.tinymceOptions`; change the
merge logic where TinyMCE options are composed so that
`rteSetup.tinymceOptions.toolbar` and `.menubar` take precedence if present, and
only fall back to `isRTE` when those keys are undefined or null. Update the code
that builds the final options (look for `toolbar`, `menubar`, `isRTE`, and
`rteSetup.tinymceOptions` in this file) to conditionally use the existing
`rteSetup.tinymceOptions` values instead of unconditionally setting them to
`isRTE`.
---
Nitpick comments:
In `@ui/guest/src/controls/rte.ts`:
- Line 124: The object literal currently uses the literal key "setupId" instead
of the resolved variable value, so replace the literal key with a computed
property using the setupId variable (e.g., change setupId: to [setupId]:)
wherever the setup map is built in rte.ts so the actual id is used as the map
key (look for the map/object creation referencing setupId in the RTE setup
code).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6a5b4ec-eb8d-4ed9-b570-7b146dc5fa81
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/lib/validators.tsui/guest/src/controls/rte.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
503-516: Makemessagesnon-optional (or guard it) to keep the contract consistent.At Line 515,
messages.push(...)is unconditional, butmessagesis typed as optional. It works with current callers, but this is fragile for future reuse.♻️ Proposed cleanup
-const rteValidator = (field: ContentTypeField, currentValue: string, messages?: FieldValidityMessage[]): boolean => { +const rteValidator = (field: ContentTypeField, currentValue: string, messages: FieldValidityMessage[]): boolean => { if (nou(field)) return false; const isRequired = isFieldRequired(field); if (!isRequired) return true; let isValid = true; if (isRequired) { const aux = document.createElement('div'); aux.innerHTML = currentValue; const trimmedContent = aux.innerText.trim(); // Get only the text and remove white space isValid = trimmedContent !== ''; if (!isValid) { messages.push(defineMessage({ defaultMessage: 'This field is required.' })); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 503 - 516, The rteValidator function declares messages?: FieldValidityMessage[] but calls messages.push(...) unguarded; either make the messages parameter required (remove the ? on messages in rteValidator signature) or add a guard before pushing (e.g., ensure messages is initialized as an array or return/skip pushing when undefined). Update the function signature or insert a check around the messages.push call (referencing rteValidator and the messages parameter) so the contract is consistent and no runtime errors occur when messages is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/utils/system.ts`:
- Around line 200-210: The loadAceEditorAssets function can append duplicate
<script> and <link> tags when called concurrently; add idempotency by
introducing a module-level sentinel (e.g., let aceLoadPromise or let
isAceLoading) and check for existing DOM nodes by id (e.g., 'ace-script' and
'ace-styles') before creating/appending elements, and have the function
set/return the sentinel Promise while the script is loading so subsequent
callers reuse it; also keep the original window.ace check so if window.ace
exists you return/resolve immediately. Ensure you reference loadAceEditorAssets,
window.ace, and use stable element ids like 'ace-script' and 'ace-styles'.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-516: The rteValidator function declares messages?:
FieldValidityMessage[] but calls messages.push(...) unguarded; either make the
messages parameter required (remove the ? on messages in rteValidator signature)
or add a guard before pushing (e.g., ensure messages is initialized as an array
or return/skip pushing when undefined). Update the function signature or insert
a check around the messages.push call (referencing rteValidator and the messages
parameter) so the contract is consistent and no runtime errors occur when
messages is omitted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d942091-583e-49e1-b80d-8c825d5265f4
📒 Files selected for processing (4)
ui/app/src/components/FormsEngine/controls/RichTextEditor.tsxui/app/src/components/FormsEngine/lib/validators.tsui/app/src/utils/system.tsui/guest/src/react/ExperienceBuilder.tsx
✅ Files skipped from review due to trivial changes (1)
- ui/app/src/components/FormsEngine/controls/RichTextEditor.tsx
4 tasks
… language setting
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@static-assets/js/tinymce-plugins/ace/plugin.js`:
- Around line 20-43: The file only registers i18n for 'en','es','de','ko_KR' so
editor.translate('code-editor') will return raw keys for other CrafterCMS
locales; fix by ensuring a safe English fallback is registered for any
unrecognized locale before translations are used: detect the active locale (from
the variable/prop that drives TinyMCE language), check if tinymce.hasI18n (or
attempt to read tinymce.getLang) for that locale, and if missing call
tinymce.addI18n(currentLocale, { 'code-editor': 'Code Editor',
'toggle-code-view': 'Toggle code view mode', fullscreen: 'Fullscreen',
'exit-fullscreen': 'Exit fullscreen' }) so editor.translate('code-editor')
always returns a human-readable string; reference tinymce.addI18n and
editor.translate when making the change.
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-520: rteValidator currently calls messages.push(...) even
though messages is optional and also contains a redundant if (isRequired) block;
update rteValidator to use the null-safe form messages?.push(...) when adding
the required-field message and remove the nested if (isRequired) guard (the
function already returned when !isRequired), leaving only the DOM parsing,
trimmedContent check and messages?.push(...) so it no longer throws when
messages is undefined.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 403dafb6-7deb-43a8-bee5-fb0f298fddb6
⛔ Files ignored due to path filters (1)
static-assets/js/tinymce-plugins/ace/plugin.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (11)
static-assets/js/tinymce-plugins/ace/plugin.jsui/app/src/components/ContentTypeManagement/descriptors/controls/rte.tsui/app/src/components/FormsEngine/lib/formUtils.tsxui/app/src/components/FormsEngine/lib/validators.tsui/app/src/services/contentTypes.tsui/app/src/utils/system.tsui/guest/src/controls/rte.tsui/npm-content/build.shui/npm-content/src/tinymce/langs/de.jsui/npm-content/src/tinymce/langs/es.jsui/npm-content/src/tinymce/langs/ko_KR.js
✅ Files skipped from review due to trivial changes (3)
- ui/npm-content/src/tinymce/langs/ko_KR.js
- ui/npm-content/src/tinymce/langs/de.js
- ui/npm-content/src/tinymce/langs/es.js
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/app/src/utils/system.ts
- ui/app/src/components/ContentTypeManagement/descriptors/controls/rte.ts
- ui/app/src/components/FormsEngine/lib/formUtils.tsx
- ui/guest/src/controls/rte.ts
…o feature/7418-richTextEditor # Conflicts: # ui/guest/package.json # yarn.lock
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
503-517: ⚡ Quick winPrefer
DOMParseroverinnerHTMLfor inert HTML parsing.Even on a detached element, assigning untrusted HTML to
innerHTMLcan still trigger side effects such as<img>/<iframe>resource loads andonerror/onloadhandlers.DOMParser.parseFromString(html, 'text/html')produces an inert document where these are not executed, which is the safer pattern for "parse HTML to extract text".♻️ Proposed refactor
- const aux = document.createElement('div'); - aux.innerHTML = currentValue; - const trimmedContent = aux.innerText.trim(); // Get only the text and remove white space + const doc = new DOMParser().parseFromString(currentValue ?? '', 'text/html'); + const trimmedContent = (doc.body.textContent ?? '').trim(); // Get only the text and remove white spaceNote:
textContentdiffers slightly frominnerTextin that it does not collapse whitespace or honor CSS-driven visibility, but for an emptiness check on RTE-produced HTML the practical result is equivalent. If preserving exactinnerTextsemantics is important, append the parsed body to a detached document fragment that is never inserted into the live DOM.🤖 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 `@ui/app/src/components/FormsEngine/lib/validators.ts` around lines 503 - 517, The rteValidator function currently creates a detached div and assigns currentValue to aux.innerHTML which can trigger resource loads; replace that inert-parsing logic by using DOMParser.parseFromString(currentValue, 'text/html') to obtain a parsed document, then extract the text via document.body.textContent?.trim() (or use a detached fragment approach if you need innerText-like whitespace collapsing) to compute isValid, keep the existing messages push and return logic unchanged, and ensure you reference the same symbols (rteValidator, currentValue, messages) when modifying the implementation.
🤖 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 `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-517: rteValidator currently only enforces required but must
also enforce maxLength like inputValidator; inside rteValidator (function
rteValidator) extract maxLength via getValidationValue(field.validations,
'maxLength'), compute trimmedContent as already done, and if maxLength is
non-null and trimmedContent.length > maxLength push a validation message using
defineMessage with the provided template and return false (ensure you set
isValid = false before returning); keep the existing required check and message
behavior and add the maxLength check after computing trimmedContent.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/validators.ts`:
- Around line 503-517: The rteValidator function currently creates a detached
div and assigns currentValue to aux.innerHTML which can trigger resource loads;
replace that inert-parsing logic by using
DOMParser.parseFromString(currentValue, 'text/html') to obtain a parsed
document, then extract the text via document.body.textContent?.trim() (or use a
detached fragment approach if you need innerText-like whitespace collapsing) to
compute isValid, keep the existing messages push and return logic unchanged, and
ensure you reference the same symbols (rteValidator, currentValue, messages)
when modifying the implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d583732b-97aa-455f-b29f-0b5bded28781
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/lib/validators.ts
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
1 participant
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.
craftercms/craftercms#7418
craftercms/craftercms#8635 - RTE language sync with studio
Summary by CodeRabbit
New Features
Bug Fixes & Improvements