Repository navigation
fix/enhanced-smart-editor - #158
NethmikaKekuu wants to merge 22 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the custom contenteditable editor with a Tiptap-based implementation in SmartEditor.tsx and rewrites the PDF generation logic in pdfUtils.ts to achieve layout parity. However, several critical issues were identified in the review: cleanPastedHtml still relies on regex and incorrectly strips variable pill spans; asynchronous image loading in pdfUtils.ts leads to a missing logo in the generated PDF; using indexOf inside the line-processing loop introduces correctness bugs and O(N^2) performance degradation; and synchronous layout calculations on every keystroke in onUpdate cause layout thrashing and typing lag.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function cleanPastedHtml(raw: string): string { | ||
| return raw | ||
| .replace(/<b\s+id="docs-internal-guid[^"]*"[^>]*>([\s\S]*?)<\/b>/gi, '$1') | ||
| .replace(/<b\s+style="[^"]*font-weight:\s*normal[^"]*"[^>]*>([\s\S]*?)<\/b>/gi, '$1') | ||
| .replace(/<meta[^>]*>/gi, '') | ||
| .replace(/<style[^>]*>[\s\S]*?<\/style>/gi, '') | ||
| .replace(/\s*id="docs-internal-guid[^"]*"/gi, '') | ||
| .replace(/<span([^>]*)font-weight\s*:\s*(?:bold|700|800|600)([^>]*)>([\s\S]*?)<\/span>/gi, '<strong>$3</strong>') | ||
| .replace(/<span([^>]*)font-style\s*:\s*italic([^>]*)>([\s\S]*?)<\/span>/gi, '<em>$3</em>') | ||
| .replace(/<span([^>]*)text-decoration\s*:[^;]*underline([^>]*)>([\s\S]*?)<\/span>/gi, '<u>$3</u>') | ||
| .replace(/<span[^>]*>([\s\S]*?)<\/span>/gi, '$1') | ||
| .replace(/[ \t]{2,}/g, ' '); | ||
| } |
There was a problem hiding this comment.
The PR description states that cleanPastedHtml() was rewritten to operate on a parsed DOM tree instead of regex to prevent destroying the data-code (or data-variable) attributes on variable pills. However, the implemented code is still entirely regex-based and contains .replace(/\u003cspan[^\u003e]*\u003e([\s\S]*?)\u003c\/span\u003e/gi, '$1'), which will strip all spans (including variable pills) and convert them to plain text. Please implement the DOM-parsing approach as described to prevent this regression.
|
/gemini reveiw |
|
It looks like you're trying to trigger a code review with |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the SmartEditor component to use the Tiptap editor framework instead of a custom contenteditable implementation, adding custom nodes for variable pills and page breaks, and updates the PDF generation utility (pdfUtils.ts) to align with the new editor layout. Feedback on the changes highlights a bug in pdfUtils.ts where loading the logo image synchronously inside the page loop fails in browser environments, recommending pre-loading it asynchronously as a base64 data URL instead. Additionally, the reviewer suggests replacing global window custom events in SmartEditor.tsx with a ref-based approach to avoid conflicts between multiple editor instances, along with minor cleanups such as removing unused event listeners and an empty JSX expression container.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the rich text editing and PDF generation system by replacing the basic contenteditable editor with a Tiptap-based SmartEditor and aligning the jsPDF rendering logic in pdfUtils.ts to match the editor's layout and page-break preview. Key feedback includes addressing a missing dependency in the editor's markdown syncing useEffect, resolving a character sanitization issue in pdfUtils.ts that strips non-Latin-1 characters (preventing Sinhala/Tamil rendering), improving type safety by replacing any with NodeViewProps, eliminating duplicated layout constants, refactoring indirect window event listeners for paste/drop actions into direct editor commands, and avoiding hardcoded configuration values in the footer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| handlePaste(_view, event) { | ||
| event.preventDefault(); | ||
| const cd = event.clipboardData; | ||
| if (!cd) return false; | ||
| const htmlData = cd.getData('text/html'); | ||
| if (htmlData) { | ||
| const md = htmlToMarkdown(cleanPastedHtml(htmlData), placeholdersRef.current); | ||
| window.dispatchEvent(new CustomEvent('smarteditor:insert', { | ||
| detail: { html: markdownToHtml(md, placeholdersRef.current) } | ||
| })); | ||
| return true; | ||
| } | ||
| const textData = cd.getData('text/plain'); | ||
| if (textData) { | ||
| const finalHtml = textData | ||
| .split(/\r?\n/) | ||
| .map(l => l.trim() ? `<p>${applyInlineMarkdown(l)}</p>` : '<p></p>') | ||
| .join(''); | ||
| window.dispatchEvent(new CustomEvent('smarteditor:insert', { detail: { html: finalHtml } })); | ||
| return true; | ||
| } | ||
| return false; | ||
| }, |
There was a problem hiding this comment.
The handlePaste logic uses a custom window event (smarteditor:insert) to insert content, which is then handled by a useEffect listener. This approach is indirect and not idiomatic for a React component. Since the editor instance is available in this scope, you can call its commands directly for a simpler and more direct implementation. This would also remove the need for the corresponding useEffect listeners.
handlePaste(_view, event) {
if (!editor) return false;
event.preventDefault();
const cd = event.clipboardData;
if (!cd) return false;
let htmlToInsert = '';
const htmlData = cd.getData('text/html');
if (htmlData) {
const md = htmlToMarkdown(cleanPastedHtml(htmlData), placeholdersRef.current);
htmlToInsert = markdownToHtml(md, placeholdersRef.current);
} else {
const textData = cd.getData('text/plain');
if (textData) {
htmlToInsert = textData
.split(/\r?\n/)
.map(l => l.trim() ? `<p>${applyInlineMarkdown(l)}</p>` : '<p></p>')
.join('');
}
}
if (htmlToInsert) {
editor.commands.insertContent(htmlToInsert, { parseOptions: { preserveWhitespace: 'full' } });
return true;
}
return false;
},
…directly and updates the useEffect dependency array.
Enhancing the Text Editor UI
Fixes #154
Description
Resolved multiple issues with content quality when pasting from Google Docs into the SmartEditor, fixed inline formatting rendering in the PDF, added a live page boundary indicator in the editor so users can see where PDF pages will break while editing, and fixed two regressions in the variable/template workflow that surfaced while testing the paste-cleanup changes.
Changes
SmartEditor.tsxcleanPastedHtml()to strip Google Docs wrapper tags (docs-internal-guid) that caused entire documents to appear bold after pastefont-weight:700,font-style:italic,text-decoration:underline) to semantic HTML tags on pastewrap()inhtmlToMarkdown()to drop bold/italic markers when they wrap multi-line content, preventing literal**stars appearing at the start and end of pasted documentscleanPastedHtml()to operate on a parsed DOM tree instead of regex over the raw HTML string. The previous regex-based span stripping matched on the first closing</span>it found, which incorrectly unwrapped the inner label/remove-button spans of a variable pill instead of the pill's own outer wrapper, silently destroying thedata-codeattribute and turning the pill into plain text (e.g.{{receiver_position}}becameReceiver Position×). Copying or pasting any paragraph containing a pill — including duplicating content within or between templates — permanently broke that variable. The new version walks the DOM, leaves anything inside[data-code]untouched, and only converts or unwraps ordinary spansPageBreakcustom TipTap node with visible dashed line rendering in the editor and<!--PAGE_BREAK-->serialisation to markdownupdatePageLines()which injects dashed visual boundary lines into the scroll container div (not ProseMirror) at calculated PDF page boundaries. Page height in pixels is derived at runtime from the editor's actual renderedoffsetWidthdivided by the known PDF content width (172mm =PAGE_W(210) - MARGIN(19)*2), multiplied by the usable PDF page height (232mm =BOTTOM_LIMIT(267) - CONTENT_START_Y(35)). This avoids hardcoding a DPI assumption and gives accurate placement on any screen resolutionupdatePageLines()so boundary lines reposition correctly when the editor panel is resizedpdf-page-lineclass and are excluded fromhtmlToMarkdown()serialisation so they never appear in saved content or the PDFTemplates.tsxsaveTemplate()blanking the editor immediately after a successful save. The success handler only fell back to the locally-generated markdown when the API response'scontentfield was exactlyundefined; if the backend instead returnedcontent: null, or omitted/reshaped the field, that check passed through andselectedTemplate.contentbecamenull. Thatnullthen propagated intoSmartEditoras theinitialMarkdownprop, whose sync effect treats falsy markdown as empty and force-reset the live editor to blank right after the save completed — for both newly-created and edited templates. The handler now always overlays the known-good markdown onto the saved template instead of trusting whatever shape the API echoes backpdfUtils.ts<!--PAGE_BREAK-->detection in the main render loop to calldoc.addPage()and resetcursorYjustifyalignment support throughout — regex patterns, type definitions, andgetAlignX()now acceptjustifyas a valid alignment valuerenderRowJustified()for proper full-justification rendering — spaces between words are expanded so each non-last line fills the full content widthunderline: falseinstead ofitalic: trueonmatch[3]sanitizeForPDF()applied at three levels — on the full markdown string after variable replacement, insideparseInlineSegments()before parsing, and per-token insiderenderRow()beforedoc.text()— to prevent jsPDF latin1 encoding errors from corrupting character spacingsegmentsToTokens()to split on unicode whitespace (\u00A0) in addition to regular spaces, preventing first/last name merging from database values that use non-breaking spacesRelated Issues
**stars appearing at start and end of pasted contentBimalSilvainstead ofBimal Silva)Testing
**stars appear at the beginning or end