-
-
Notifications
You must be signed in to change notification settings - Fork 18
Fix: Remove trailing paragraph from reports stored in database #2201
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
base: stage
Are you sure you want to change the base?
Conversation
| "machine.context.reviewData.visualEditor.content": | ||
| newContent, | ||
| }, | ||
| $unset: { |
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.
issue: while safe, this unconditionally unsets fields that may not exist. Consider checking first
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.
I added field validations before unsetting the data.
| const schema = editorParser.editor2schema( | ||
| event.reviewData.visualEditor.toJSON() | ||
| const visualEditorJSON = event.reviewData.visualEditor.toJSON(); | ||
| const cleanedVisualEditor = editorParser.removeTrailingParagraph( |
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.
question: Is there a way to prevent the <p> from being added instead of having to remove them afterward?
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.
I investigated alternatives, but couldn’t find a native way to prevent the persistence of the trailing paragraph. The TrailingNodeExtension is required for the UX (it allows insertion at the end), but it does not provide an option to exclude it during serialization.
|
| @@ -0,0 +1,171 @@ | |||
| import { Db } from "mongodb"; | |||
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.
I advise against this data migration to not risk corrupting the existing data.
The trailing space is not an issue for published reports and we could just let the new ones to be created appropriately.
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.
Thanks for the feedback — your concern about the risk of modifying existing data totally makes sense.
Just to provide a bit more context from the previous fixes:
in PR #2160, I fixed a structural issue in the editor by ensuring that the Remirror extensions — especially TrailingNodeExtension — were always properly initialized. Previously, extension instances were being reused and could become inconsistent or overwritten after document mutations, which led to unexpected behavior.
Then, in PR #2173, I added a frontend sanitization step that removed the trailing <p> from the content before passing it to the editor. Since TrailingNodeExtension already adds this node automatically, having an extra <p> caused conflicts when inserting new nodes into the document.
This solved the issue at runtime, but it also made the editor depend on this cleanup step every time data was loaded. To avoid relying on this permanent frontend sanitization, I considered adding a migration to normalize the data at the source, so both new and existing reports would be consistent and I could remove that extra logic from the editor.
That said, I agree that if the extra <p> doesn’t impact already published reports, it might not be worth the risk of modifying existing data. In that case, I can simply keep the fix to prevent the <p> from being persisted in new reports and continue handling older ones in the frontend, as is currently done.
How do you think we should proceed?



Description
This PR addresses the technical debt tracked in the issue by fixing the root cause of trailing
<p>paragraphs being persisted in report editor content stored in the database.Previously, the frontend had to apply a temporary workaround (e.g.,
removeTrailingParagraph) to sanitize API responses before loading them into the editor, because some reports were saved with an extra trailing<p>node. This PR prevents saving reports with trailing paragraphs at the persistence layer and includes a data cleanup to normalize existing records.As a result:
TrailingNodeExtensionRelated Ticket #2179
Type of change
Testing
To validate this change:
Backend / Data validation
<p>paragraph.Backward compatibility
<p>elements).Editor regression checks (core workflow)
Impacted scenarios:
TrailingNodeExtensionand insertion logic for new nodesNo special build is required beyond the standard environment. If a migration/script is included, ensure it runs in the target environment before validating.
Developer Checklist
General
console.logor related logging is added.Frontend Changes
Backend Changes
Tests
Test IDs
Merge Request Review Checklist