[8320] RTE fails silently on initialization in XB when <!DOCTYPE html> is missing#4809
Open
jvega190 wants to merge 2 commits intocraftercms:developfrom
Open
[8320] RTE fails silently on initialization in XB when <!DOCTYPE html> is missing#4809jvega190 wants to merge 2 commits intocraftercms:developfrom
jvega190 wants to merge 2 commits intocraftercms:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a missing-doctype runtime guard to Rich Text Editor initialization: detects absent Changes
Sequence Diagram(s)sequenceDiagram
participant RTE as initTinyMCE
participant Doc as document
participant Logger as Logger
participant Snack as SnackService
participant Lock as LockService
participant Rx as RxJS
RTE->>Doc: check document.doctype
alt doctype present
RTE->>RTE: continue TinyMCE init flow
else doctype missing
RTE->>Logger: log error "no doctype"
RTE->>Snack: show `validations.noDocTypeError`
RTE->>Lock: unlock current item
RTE->>Rx: return NEVER (abort initialization)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/guest/src/controls/rte.ts (1)
118-128: ReplaceNEVERwithEMPTYin the error path to allow the subscription to complete properly.In the normal flow, the observable completes when the editor is closed (line 289:
dispatch$.complete()). However, the error condition at line 127 returnsNEVER, which never emits, errors, or completes. Since this function is consumed viaswitchMapin a Redux-Observable epic (root.ts, line 610), returningNEVERleaves the subscription open indefinitely instead of signaling completion. ReturningEMPTYcompletes immediately, maintaining the semantic consistency of the observable contract.♻️ Suggested change
-import { NEVER, Observable, Subject } from 'rxjs'; +import { EMPTY, NEVER, Observable, Subject } from 'rxjs';- return NEVER; + return EMPTY;🤖 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 118 - 128, The error branch returns RxJS constant NEVER which never completes; replace it with EMPTY so the observable completes immediately; update the code in the block that checks nou(document.doctype) (where snackGuestMessage and unlockItem({ path }) are posted) to return EMPTY instead of NEVER and ensure EMPTY is imported from 'rxjs' so the epic consuming this via switchMap can complete correctly.
🤖 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 118-128: The guard that checks document.doctype and returns NEVER
must be moved to the top of the function before any DOM mutations (before code
that uses isRecordElInline, before hiding recordEl, inserting blockEl, and
before calling record.element.classList.remove(emptyFieldClass)); update the
function so the doctype check runs first and triggers the error log,
snackGuestMessage post, post(unlockItem({ path })), and return NEVER immediately
(so cancel() wired in tinymce setup is not needed), ensuring no DOM changes
occur when document.doctype is absent.
---
Nitpick comments:
In `@ui/guest/src/controls/rte.ts`:
- Around line 118-128: The error branch returns RxJS constant NEVER which never
completes; replace it with EMPTY so the observable completes immediately; update
the code in the block that checks nou(document.doctype) (where snackGuestMessage
and unlockItem({ path }) are posted) to return EMPTY instead of NEVER and ensure
EMPTY is imported from 'rxjs' so the epic consuming this via switchMap can
complete correctly.
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#8320
Summary by CodeRabbit