fix(slides): stop false content-loss confirms in both editors, and the data loss behind them - #2395
Open
samark231 wants to merge 12 commits into
Open
fix(slides): stop false content-loss confirms in both editors, and the data loss behind them#2395samark231 wants to merge 12 commits into
samark231 wants to merge 12 commits into
Conversation
…r deleting the block
Deploying frontend-admin-dashboard with
|
| Latest commit: |
256498c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5b259f8f.frontend-admin-dashboard.pages.dev |
| Branch Preview URL: | https://fix-lexical-callout-content.frontend-admin-dashboard.pages.dev |
Contributor
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
| CodeStyle | 2 minor |
🟢 Metrics 60 complexity
Metric Results Complexity 60
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…zing it at creation
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
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.
Summary of Changes
Authors were getting
"To prevent accidental data loss… This will remove 1 callout / 1 table from the slide."confirms on slides where they had deleted nothing. The backend guard was arithmetically right about the payload it received — the editor really was dropping those blocks on the way through, while the words stayed visible on screen, so there was nothing for the author to act on. Two of the causes were silent data loss, not just a noisy dialog.Root causes fixed:
textContent, so a table or image nested in a callout was flattened to its words — the<table>/<img>vanished from the saved HTML while every cell's text stayed on screen.unwrapMediaWrappersreplaceWith-es any div wrapping a lone image/iframe/video, including a custom block's own marker div — block, theme and text went with it.formatHTMLString's empty-image stripper had the same shape and could delete a callout whose upload had failed.<div>. Hand- and AI-authored lesson HTML puts it on<aside>/<blockquote>, which imported as a plain paragraph and never emitted the marker again.<img>counted as content. An abandoned upload placeholder is dropped by the importer and stripped byformatHTMLStringon every save, so it read as "1 image removed" on saves that changed nothing.Backend:
SlideService.structuralMarkerCountscounts only images with a usablesrc(newcountRenderableImages); empty/null/undefinedplaceholders are ignored. Real image loss is still reported.IMG_TAGmatches quoted attribute values properly, so analt="a > b"can't cut a tag short before itssrc.SlideStructuralLossTest(placeholder ignored, real loss alongside a placeholder still caught, quote-style/spacing variants).Frontend:
CalloutBlockstores rich HTML instead of plaintext; editing UI moves from<textarea>to the sharedRichTextField. Legacy plaintext callouts round-trip byte-identically; legacy multi-line ones get a one-time\n→<br>promotion (they also collapsed on the learner side, which skips newline conversion inside[data-yoopta-type]subtrees).CalloutBlock.importTagswidened todiv/aside/blockquote/section.importMatchreturns null without the marker, so plain elements of those tags are untouched.unwrapMediaWrappersskips wrappers carrying a block marker or their own text; genuine Yoopta flex wrappers still unwrap.formatHTMLString's empty-image stripper skips divs carryingdata-yoopta-type.normalizeYooptaHtmlcarries markup (not just words) out of a legacy<dl>callout, so bold/links/nested tables survive conversion.structuralCountsignores src-less images, kept in step with the backend rule.stashDocDraftLocallyrefuses to write a local draft while that slide's load is flagged lossy.Legacy (Yoopta) editor — the mid-session "will remove 1 table" confirm
Reported symptom: mid-session, on a slide where the author added or removed no table,
Save draft says "This will remove 1 table from the slide."
The editor's existing save-side checks only fire when more than half the blocks vanish, or
when a serializer throws. A serialize that quietly drops one table out of twenty passes both,
so the payload reaches the server, and the backend's structural guard is the first thing to
notice — where the only thing it can say is "you are removing a table", which to the author is
simply false.
New
detectSerializeLoss(editorValue, serializedHtml)closes that gap: it compares the editorvalue against its own serialization per structural type (table / image / video-embed / mermaid
/ each custom block). A mismatch means the serializer lost a block the editor still holds, so the
save is flagged degraded and refused with the honest "this slide could not be read correctly"
message instead of a delete-confirm the author cannot act on. The lossy HTML is also no longer
cached as the last-known-good baseline.
It cannot fire on a real deletion: deleting a block removes it from the editor value too, so both
sides drop together. It compares one snapshot against itself — no debounce, no older baseline, no
timing window.
Measured on real production content (local DB restored from prod dumps):
appReloadPreprocess→html.deserialize→ the app's serialize →formatHTMLString) and scored with the backend'sexact counting: 0 would trip the confirm from a plain open→save, 126 clean, 6 caught by the
existing load-time guard. So the drop is transient/session-dependent, which is exactly what the
new check is shaped to catch.
detectSerializeLossacross the 500 richest real Yoopta slides(498 clean; the 2 that threw are the already-handled "serializer throws" path).
Also found and not fixed here (separate issues, no code change in this PR):
html.deserializeitself — 4 of them tables, all AI-generated orpasted markup. Those slides are effectively unsavable today: the loss happens during load, so
the author is told to reload, which reproduces it. Needs its own bisection.
Ctrl+Btoggles the sidebar (shadcn'sSIDEBAR_KEYBOARD_SHORTCUT,sidebar.tsx:18) on awindow-level listener, so it steals the bold shortcut inside the editor.
Related Issue
Follow-up to the slide content-loss work (
docs/SLIDE_CONTENT_LOSS_INVESTIGATION.md).Type of Change
How Has This Been Tested?
lexical-editor/roundtrip-integrity/test.ts), one per reproduced false alarm. Counts are asserted for equality, so a duplicated table fails as loudly as a dropped one. Includes blast-radius pins: an image inside a text-carrying wrapper still imports, a plain<blockquote>stays a quote, plain<aside>/<section>are untouched, and the<br>promotion is idempotent.SlideStructuralLossTest;tsc --noEmitclean;design-lint0 errors.admin_coreon the Docker DB): unchanged content → 200; callout genuinely deleted → 409; table inside a callout flattened, i.e. the old editor's exact output → 409"This will remove 1 table from the slide."(reproduces the reported popup); placeholder image dropped → 200; real image dropped → 409.detectSerializeLoss(including "never fires on a real deletion" and the placeholder-media cases); 105 tests pass across the slides area; false-positive sweep over 500 real slides reported zero firings.<textarea>to a contentEditable, and while it now uses the sameRichTextField-inside-BlockChromepattern already shipped for quiz options, nobody has clicked it. Worth doing before merge.Checklist