[PE-45] feat: page export as PDF & Markdown #5705
Conversation
WalkthroughThe changes introduce several enhancements, including updates to styling variables, new constants for modal widths, the addition of a PDF document rendering component, and the implementation of an export functionality for pages. New helper functions for processing content and converting units are also included. Additionally, the project now includes new dependencies for PDF rendering, expanding its capabilities. Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (19)
web/core/components/editor/index.ts (1)
1-3: LGTM! Consider alphabetical ordering for consistency.The addition of the PDF module export aligns well with the PR objectives, introducing the ability to export pages as PDF. This change effectively makes the PDF-related functionality available for import alongside the existing editor components.
For improved readability and easier maintenance, consider ordering the exports alphabetically:
export * from "./lite-text-editor"; -export * from "./pdf"; export * from "./rich-text-editor"; +export * from "./pdf";packages/ui/src/modals/constants.ts (1)
7-9: LGTM! Consider reordering for consistency.The addition of
SM,MD, andLGconstants to theEModalWidthenum is a good improvement. It provides more granular control over modal widths and follows Tailwind CSS conventions.For better readability and consistency, consider reordering the enum values from smallest to largest. Here's a suggested order:
export enum EModalWidth { SM = "sm:max-w-sm", MD = "sm:max-w-md", LG = "sm:max-w-lg", XL = "sm:max-w-xl", XXL = "sm:max-w-2xl", XXXL = "sm:max-w-3xl", XXXXL = "sm:max-w-4xl", VXL = "sm:max-w-5xl", VIXL = "sm:max-w-6xl", }This ordering aligns with the natural progression of sizes and makes it easier to locate specific widths at a glance.
web/helpers/file.helper.ts (2)
1-6: Enhance function documentation for clarity.The current documentation is good, but it could be more detailed to improve clarity and maintainability.
Consider expanding the documentation as follows:
/** * Fetches an image from the provided URL and converts it to a base64-encoded string. * * @param {string} url - The URL of the image to be converted. * @returns {Promise<string>} A promise that resolves with the base64-encoded string representation of the image. * @throws {Error} If the image fetch fails or if the conversion process encounters an error. */
7-13: Approve image fetching with a suggestion for improved error handling.The image fetching logic is well-implemented. The use of the Fetch API and the conversion to a blob are appropriate for this task.
Consider enhancing the error handling by including the HTTP status code in the error message. This can be achieved by modifying line 10 as follows:
throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`);This change will provide more detailed information for debugging purposes.
web/helpers/common.helper.ts (1)
40-41: LGTM with suggestions for improvementThe
convertRemToPixelfunction is well-implemented and typed correctly. However, I have a few suggestions to enhance its clarity and flexibility:
Consider adding a comment to explain the purpose of the 0.9 scaling factor. This will help other developers understand the rationale behind this specific conversion.
To make the function more flexible, you might want to consider making the scaling factor configurable. This could be achieved by adding an optional parameter with a default value of 0.9.
Here's an example of how you could refactor the function:
/** * Converts rem units to pixels. * @param rem The value in rem units to convert. * @param scaleFactor Optional scaling factor (default: 0.9) to adjust the conversion. * @returns The equivalent value in pixels. */ export const convertRemToPixel = (rem: number, scaleFactor: number = 0.9): number => rem * scaleFactor * 16;This refactoring maintains the current behavior while allowing for future flexibility if needed.
web/core/components/editor/pdf/document.tsx (3)
8-30: LGTM: Comprehensive font registration. Consider lazy loading for optimization.The Inter font family is well-registered with a wide range of weights and styles, which will enhance the visual consistency of the PDF output.
For optimization, consider lazy loading the fonts or only registering the weights and styles that are actually used in the PDF content. This could potentially improve the initial load time of the component.
32-35: LGTM: Props type is well-defined. Consider adding content validation.The Props type is correctly defined with appropriate types for content and pageFormat.
For improved type safety and to prevent potential runtime errors, consider adding a runtime check or using a library like zod to validate the content string. This could help ensure that the content is valid HTML before attempting to render it in the PDF.
37-53: LGTM: PDFDocument component is well-structured. Consider adding error handling.The PDFDocument component is correctly implemented using @react-pdf/renderer and react-pdf-html. The structure is clean and follows React best practices.
Consider the following improvements:
- Add error handling for cases where the HTML content might be invalid or cause rendering issues.
- Implement a loading state or fallback UI while the PDF is being generated, especially for large documents.
- Consider making the padding and background color customizable through props if this component might be reused in different contexts.
Example implementation for error handling:
export const PDFDocument: React.FC<Props> = (props) => { const { content, pageFormat } = props; return ( <ErrorBoundary fallback={<ErrorFallback />}> <Document> <Page size={pageFormat} style={{ backgroundColor: "#ffffff", padding: 64, }} > <Html stylesheet={EDITOR_PDF_DOCUMENT_STYLESHEET}>{content}</Html> </Page> </Document> </ErrorBoundary> ); };Don't forget to implement the ErrorBoundary and ErrorFallback components.
web/core/components/pages/editor/title.tsx (1)
40-41: Improved styling approach for TextArea componentThe changes to the TextArea component are good improvements:
- Removing the inline style in favor of utility classes is a better practice for maintainability and consistency.
- The added utility classes provide more specific styling control.
These changes align well with modern CSS practices and improve the component's flexibility.
Consider extracting the long className string into a separate constant for better readability:
+ const textAreaClassName = cn( + titleClassName, + "w-full outline-none p-0 border-none resize-none rounded-none" + ); <TextArea - className={cn(titleClassName, "w-full outline-none p-0 border-none resize-none rounded-none")} + className={textAreaClassName} placeholder="Untitled" // ... other props />This change would make the JSX more readable and the className easier to maintain.
web/core/components/pages/editor/header/options-dropdown.tsx (2)
166-172: LGTM: New export menu item added.The new export menu item is well-structured and consistent with other items in the
MENU_ITEMSarray. The icon choice and action are appropriate for the export functionality.Consider adding a condition to
shouldRenderbased on whether exporting is allowed for the current user or page state. This would make the menu item's visibility consistent with other conditional items like "lock-unlock-page".
176-183: LGTM: Export modal integration.The
ExportPageModalcomponent is well-integrated into the existing structure. Its placement and props are appropriate, and the use of a fragment to wrap multiple elements is correct.Consider memoizing the
onClosecallback usinguseCallbackto optimize performance, especially if this component re-renders frequently:const onCloseExportModal = useCallback(() => setIsExportModalOpen(false), []);Then use
onCloseExportModalin theonCloseprop ofExportPageModal.web/core/constants/editor.ts (4)
138-148: LGTM: Well-defined font family styles.The
EDITOR_PDF_FONT_FAMILY_STYLESconstant provides a consistent typography across the PDF document. However, consider using a fallback font stack for better cross-platform compatibility.Consider updating the font family definitions to include fallback fonts:
const EDITOR_PDF_FONT_FAMILY_STYLES: Styles = { "*:not(.courier, .courier-bold)": { - fontFamily: "Inter", + fontFamily: "Inter, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif", }, ".courier": { - fontFamily: "Courier", + fontFamily: "Courier, monospace", }, ".courier-bold": { - fontFamily: "Courier-Bold", + fontFamily: "Courier-Bold, Courier, monospace", }, };
150-203: LGTM: Well-structured typography styles.The
EDITOR_PDF_TYPOGRAPHY_STYLESconstant provides a clear hierarchy for headings and paragraphs. The use ofconvertRemToPixelensures consistent sizing.Consider adding
lineHeightproperties to improve readability and accessibility:const EDITOR_PDF_TYPOGRAPHY_STYLES: Styles = { // page title "h1.page-title": { fontSize: convertRemToPixel(1.6), fontWeight: "bold", marginTop: 0, marginBottom: convertRemToPixel(2), + lineHeight: 1.2, }, // headings "h1:not(.page-title)": { fontSize: convertRemToPixel(1.4), fontWeight: "semibold", marginTop: convertRemToPixel(2), marginBottom: convertRemToPixel(0.25), + lineHeight: 1.2, }, // ... (apply similar changes to other headings) // paragraph "p:not(table p)": { fontSize: convertRemToPixel(0.8), + lineHeight: 1.5, }, // ... (rest of the styles) };
205-243: LGTM: Comprehensive list styles with task list support.The
EDITOR_PDF_LIST_STYLESconstant provides well-defined styles for various list types, including task lists with checkboxes.For consistency with other styles, consider using
convertRemToPixelfor pixel values:const EDITOR_PDF_LIST_STYLES: Styles = { "ul, ol": { fontSize: convertRemToPixel(0.8), - marginHorizontal: -20, + marginHorizontal: convertRemToPixel(-1.25), // Assuming 1rem = 16px }, // ... (rest of the styles) "div.input-checkbox": { // ... (other properties) - borderWidth: "1.5px", + borderWidth: convertRemToPixel(0.09375), // 1.5px }, // ... (rest of the styles) };
268-313: LGTM: Comprehensive PDF document stylesheet.The
EDITOR_PDF_DOCUMENT_STYLESHEETeffectively combines all previously defined styles and adds necessary styles for additional elements.Consider the following improvements for consistency and accessibility:
- Use
convertRemToPixelfor all pixel values:export const EDITOR_PDF_DOCUMENT_STYLESHEET = StyleSheet.create({ // ... (other styles) blockquote: { - borderLeft: "3px solid gray", + borderLeft: `${convertRemToPixel(0.1875)} solid gray`, // ... (other properties) }, // ... (other styles) "div[data-type='horizontalRule']": { // ... (other properties) - height: 1, + height: convertRemToPixel(0.0625), // ... (other properties) }, // ... (other styles) });
- Improve color contrast for better accessibility:
export const EDITOR_PDF_DOCUMENT_STYLESHEET = StyleSheet.create({ // ... (other styles) "[data-node-type='mention-block']": { margin: 0, - color: "#3f76ff", - backgroundColor: "#3f76ff33", + color: "#0056b3", // Darker blue for better contrast + backgroundColor: "#e6f2ff", // Lighter background paddingHorizontal: convertRemToPixel(0.375), }, // ... (other styles) });These changes will improve consistency in using the
convertRemToPixelhelper and enhance accessibility through better color contrast.packages/editor/src/styles/editor.css (1)
47-47: LGTM! Consider adding system fonts to the font stack.The change to use "Inter" as the primary font for the sans-serif style is a good improvement. It aligns with the PR objective of updating the default
font-familyacross all editors and provides a modern, readable font designed for screens.Consider expanding the font stack to include system fonts for better performance and fallback options:
--- --font-style: "Inter", sans-serif; +++ --font-style: "Inter", -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Helvetica, Arial, sans-serif;This change ensures that if "Inter" is not available, the browser will use an appropriate system font before falling back to a generic sans-serif font. This can improve performance by potentially avoiding an additional font download and provides a more consistent look across different operating systems.
web/core/components/pages/modals/export-page-modal.tsx (3)
166-171: Useelse ifto prevent unnecessary checksSince
selectedExportFormatcan only be either"pdf"or"markdown", usingelse ifafter the first condition avoids an unnecessary check when the first condition is true.Update the conditional statements:
if (selectedExportFormat === "pdf") { await handleExportAsPDF(); - } - if (selectedExportFormat === "markdown") { + } else if (selectedExportFormat === "markdown") { await handleExportAsMarkdown(); }
257-257: Simplifykeyprop inCustomSelect.OptionIn the
PAGE_FORMATSoptions, thekeyprop usesformat.key.toString(). Sinceformat.keyis already a string, callingtoString()is unnecessary.Simplify the
keyprop:- <CustomSelect.Option key={format.key.toString()} value={format.key}> + <CustomSelect.Option key={format.key} value={format.key}>
116-119: Clarify the use ofsetTimeoutwhen resetting the formThe
resetfunction is called inside asetTimeoutwith a delay of 300ms. This may be related to the modal's closing animation, but it's not immediately clear. Adding a comment can improve code readability.Add a comment to explain the delay:
setTimeout(() => { reset(); + // Reset the form after the modal close animation completes }, 300);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (19)
web/public/fonts/inter/bold-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/bold.ttfis excluded by!**/*.ttfweb/public/fonts/inter/heavy-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/heavy.ttfis excluded by!**/*.ttfweb/public/fonts/inter/light-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/light.ttfis excluded by!**/*.ttfweb/public/fonts/inter/medium-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/medium.ttfis excluded by!**/*.ttfweb/public/fonts/inter/regular-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/regular.ttfis excluded by!**/*.ttfweb/public/fonts/inter/semibold-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/semibold.ttfis excluded by!**/*.ttfweb/public/fonts/inter/thin-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/thin.ttfis excluded by!**/*.ttfweb/public/fonts/inter/ultrabold-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/ultrabold.ttfis excluded by!**/*.ttfweb/public/fonts/inter/ultralight-italic.ttfis excluded by!**/*.ttfweb/public/fonts/inter/ultralight.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
- packages/editor/src/styles/editor.css (1 hunks)
- packages/ui/src/modals/constants.ts (1 hunks)
- web/core/components/editor/index.ts (1 hunks)
- web/core/components/editor/pdf/document.tsx (1 hunks)
- web/core/components/editor/pdf/index.ts (1 hunks)
- web/core/components/pages/editor/header/options-dropdown.tsx (4 hunks)
- web/core/components/pages/editor/title.tsx (2 hunks)
- web/core/components/pages/modals/export-page-modal.tsx (1 hunks)
- web/core/components/pages/modals/index.ts (1 hunks)
- web/core/constants/editor.ts (3 hunks)
- web/helpers/common.helper.ts (1 hunks)
- web/helpers/editor.helper.ts (1 hunks)
- web/helpers/file.helper.ts (1 hunks)
- web/package.json (2 hunks)
🧰 Additional context used
🪛 Biome
web/helpers/editor.helper.ts
[error] 143-143: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.(lint/correctness/noEmptyCharacterClassInRegex)
[error] 153-153: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.(lint/correctness/noEmptyCharacterClassInRegex)
🔇 Additional comments (14)
web/core/components/pages/modals/index.ts (1)
3-3: LGTM! New export aligns with PR objectives.The addition of
export * from "./export-page-modal";is consistent with the PR's goal of introducing page export functionality. The naming convention and placement within the file are appropriate.To ensure the completeness of this feature, let's verify the implementation of the exported module:
This script will help confirm that the exported module is properly implemented and contains the necessary components for the export functionality.
✅ Verification successful
Verified the addition of
export-page-modal.tsx.The exported module exists and is properly implemented with the necessary components for the export functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the export-page-modal file # Check if the file exists if [ -f "web/core/components/pages/modals/export-page-modal.ts" ] || [ -f "web/core/components/pages/modals/export-page-modal.tsx" ]; then echo "export-page-modal file exists." # Check the content of the file file_path=$(fd -t f "export-page-modal" web/core/components/pages/modals) if [ -n "$file_path" ]; then echo "File contents:" cat "$file_path" else echo "Couldn't find the exact file path." fi else echo "export-page-modal file does not exist in the expected location." fiLength of output: 9255
web/helpers/file.helper.ts (1)
14-30: Well-structured FileReader usage and Promise handling.The implementation of the FileReader and Promise handling is well-done. It correctly manages the asynchronous nature of the file reading process and includes appropriate error handling.
Key points:
- Proper use of FileReader to convert blob to base64.
- Clear Promise structure with resolve and reject handlers.
- Comprehensive error handling for both reading failures and null results.
web/core/components/editor/pdf/document.tsx (1)
1-6: LGTM: Imports and directives are correctly structured.The "use client" directive and import statements are well-organized and appropriate for the component's functionality.
web/core/components/pages/editor/title.tsx (3)
36-36: Improved styling approach for read-only titleThe changes to the read-only title rendering are good improvements:
- Removing the inline style in favor of className is a better practice for maintainability.
- Adding the "break-words" class improves text wrapping, preventing overflow issues.
These changes align well with React best practices and improve the component's flexibility.
Line range hint
1-78: Summary of changes and recommendationsThe changes in this file primarily focus on improving the styling approach:
- Removal of inline styles in favor of utility classes and classNames.
- Better handling of text wrapping for the read-only title.
- More specific styling control for the TextArea component.
These changes align well with React and CSS best practices, improving maintainability and consistency. However, there are a couple of points to consider:
- Verify the impact of removing
fontStylefromusePageFiltersas it might affect other components.- Consider extracting the long className string in the TextArea component into a separate constant for better readability.
Overall, these changes represent a positive step towards more maintainable and consistent styling in the component.
3-3: Verify the impact of removing fontStyle from usePageFiltersThe
fontStyleproperty has been removed from theusePageFiltershook. This change suggests a shift in how text styling is handled in the component.To ensure this change doesn't negatively impact the component's functionality:
Also applies to: 26-26
web/package.json (1)
35-35: New dependencies added for PDF functionality.The addition of
@react-pdf/renderer(v^3.4.5) andreact-pdf-html(v^2.1.2) aligns with the PR objective of implementing PDF export functionality. These libraries are well-known for handling PDF generation in React applications.To ensure these dependencies are being utilized correctly, let's verify their usage:
This script will help us confirm that the new dependencies are being used in the codebase and that PDF-related functionality has been implemented.
Also applies to: 61-61
web/core/components/pages/editor/header/options-dropdown.tsx (2)
3-3: LGTM: New imports for export functionality.The added imports (
useState,ArrowUpToLine, andExportPageModal) are consistent with the new export feature being implemented. They provide the necessary hooks, icons, and components for the new functionality.Also applies to: 6-6, 12-12
46-46: LGTM: New state for export modal visibility.The addition of
isExportModalOpenstate using theuseStatehook is well-implemented. It's appropriately named and initialized tofalse, which is correct for managing the visibility of the export modal.web/core/constants/editor.ts (2)
1-1: LGTM: New imports for PDF styling.The added imports from "@react-pdf/renderer" and the helper function
convertRemToPixelare appropriate for the new PDF styling functionality.Also applies to: 27-28
Line range hint
1-313: Summary: PDF styling constants align well with PR objectives.The introduced PDF styling constants and exported stylesheet in
web/core/constants/editor.tsprovide a comprehensive set of styles for PDF document generation. This aligns perfectly with the PR objective of adding PDF export functionality.Key points:
- Font family styles ensure consistent typography.
- Well-structured typography styles provide a clear hierarchy.
- Comprehensive list styles, including support for task lists.
- Code styles offer good visual distinction for code elements.
- The exported stylesheet combines all styles effectively.
The suggested improvements focus on consistency, accessibility, and addressing TODOs, which will further enhance the quality of the PDF export feature.
web/helpers/editor.helper.ts (2)
9-122: The functionreplaceCustomComponentsFromHTMLContentis well-implementedThe function effectively transforms custom HTML components into standard HTML elements suitable for PDF generation. Asynchronous operations and error handling are appropriately managed.
129-156: The functionreplaceCustomComponentsFromMarkdownContentis well-implementedThe function correctly processes Markdown content, replacing custom components with standard Markdown syntax suitable for exporting.
🧰 Tools
🪛 Biome
[error] 143-143: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.(lint/correctness/noEmptyCharacterClassInRegex)
[error] 153-153: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.(lint/correctness/noEmptyCharacterClassInRegex)
web/core/components/pages/modals/export-page-modal.tsx (1)
143-149: Handle asynchronous operations consistentlyThe
replaceCustomComponentsFromMarkdownContentfunction is called withoutawait, assuming it returns synchronously. If it returns a promise, this could lead to unexpected behavior.Confirm whether
replaceCustomComponentsFromMarkdownContentis synchronous. If it's asynchronous, update the code accordingly:- const parsedMarkdownContent = replaceCustomComponentsFromMarkdownContent({ + const parsedMarkdownContent = await replaceCustomComponentsFromMarkdownContent({
| @@ -0,0 +1 @@ | |||
| export * from "./document"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Missing document.ts Module
The index.ts file is attempting to export from ./document, but no such document.ts file exists in the web/core/components/editor/pdf/ directory. Please verify the existence of the document module or update the export path accordingly to ensure proper functionality.
🔗 Analysis chain
LGTM! Consider verifying exports from "./document".
The use of a barrel file (index.ts) is a good practice for simplifying imports and improving module organization. This change aligns well with the PR objectives of introducing PDF export functionality.
To ensure that only necessary entities are being re-exported, please verify the contents of the "./document" module. You can use the following script to check the exports:
This will help confirm that we're not inadvertently exposing any internal implementation details or creating potential naming conflicts.
Further Verification Needed: Export Module Missing
It appears that web/core/components/editor/pdf/document.ts does not exist. Please verify the existence and correct naming of the document module. You can use the following scripts to investigate:
Further Verification Needed: Corrected Export Module Checks
The previous scripts encountered issues due to incorrect command syntax. Please use the following updated scripts to verify the exports:
This will help identify the actual TypeScript files present and their export statements, ensuring that the index.ts file references the correct module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the exports from the "./document" module
# Test: Display the contents of the document.ts file
cat web/core/components/editor/pdf/document.ts
# Test: List all exported entities from the document.ts file
ast-grep --lang typescript --pattern 'export $_' web/core/components/editor/pdf/document.ts
Length of output: 649
Script:
#!/bin/bash
# Description: List all TypeScript files in the pdf directory
fd --extension ts --type f web/core/components/editor/pdf/
Length of output: 490
Script:
#!/bin/bash
# Description: Search for export statements in the pdf directory
ast-grep --lang typescript --pattern 'export $_' web/core/components/editor/pdf/
Length of output: 514
Script:
#!/bin/bash
# Description: List all TypeScript files in the pdf directory
fd --extension ts --type f . web/core/components/editor/pdf/
Length of output: 102
Script:
#!/bin/bash
# Description: Search for export statements in TypeScript files within the pdf directory
# List all TypeScript files in the pdf directory
TS_FILES=$(fd --extension ts --type f . web/core/components/editor/pdf/)
# Iterate over each TypeScript file and list export statements
for file in $TS_FILES; do
echo "Exports in $file:"
ast-grep --lang typescript --pattern 'export $_' "$file" || echo "No exports found in $file."
echo ""
done
Length of output: 916
| export const getBase64Image = async (url: string): Promise<string> => { | ||
| const response = await fetch(url); | ||
| // check if the response is OK | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch image: ${response.statusText}`); | ||
| } | ||
|
|
||
| const blob = await response.blob(); | ||
| return new Promise((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
|
|
||
| reader.onloadend = () => { | ||
| if (reader.result) { | ||
| resolve(reader.result as string); | ||
| } else { | ||
| reject(new Error("Failed to convert image to base64.")); | ||
| } | ||
| }; | ||
|
|
||
| reader.onerror = () => { | ||
| reject(new Error("Failed to read the image file.")); | ||
| }; | ||
|
|
||
| reader.readAsDataURL(blob); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation for the URL parameter.
The function is well-structured and focused. However, to improve robustness, consider adding input validation for the URL parameter.
Here's a suggested improvement:
export const getBase64Image = async (url: string): Promise<string> => {
if (!url || typeof url !== 'string') {
throw new Error('Invalid URL provided');
}
// Try to create a URL object to validate the URL
try {
new URL(url);
} catch (error) {
throw new Error('Invalid URL format');
}
// Rest of the function remains the same
...
};This addition will ensure that the function receives a valid URL string before attempting to fetch the image, potentially preventing errors further down the line.
| const EDITOR_PDF_CODE_STYLES: Styles = { | ||
| // code block | ||
| "[data-node-type='code-block']": { | ||
| marginVertical: convertRemToPixel(0.5), | ||
| padding: convertRemToPixel(1), | ||
| borderRadius: convertRemToPixel(0.5), | ||
| backgroundColor: "#f7f7f7", | ||
| fontSize: convertRemToPixel(0.7), | ||
| }, | ||
| // inline code block | ||
| // TODO: update width | ||
| "[data-node-type='inline-code-block']": { | ||
| margin: 0, | ||
| paddingVertical: convertRemToPixel(0.25 / 4 + 0.25 / 8), | ||
| paddingHorizontal: convertRemToPixel(0.375), | ||
| border: "0.5px solid #e5e5e5", | ||
| borderRadius: convertRemToPixel(0.25), | ||
| backgroundColor: "#e8e8e8", | ||
| color: "#f97316", | ||
| fontSize: convertRemToPixel(0.7), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
LGTM: Well-defined code styles with room for improvement.
The EDITOR_PDF_CODE_STYLES constant provides good visual distinction for code elements. However, there's a TODO comment that needs to be addressed.
Address the TODO comment for updating the width of inline code blocks. Consider using a flexible width approach:
const EDITOR_PDF_CODE_STYLES: Styles = {
// ... (code block styles)
// inline code block
- // TODO: update width
"[data-node-type='inline-code-block']": {
margin: 0,
paddingVertical: convertRemToPixel(0.25 / 4 + 0.25 / 8),
paddingHorizontal: convertRemToPixel(0.375),
border: "0.5px solid #e5e5e5",
borderRadius: convertRemToPixel(0.25),
backgroundColor: "#e8e8e8",
color: "#f97316",
fontSize: convertRemToPixel(0.7),
+ display: "inline-block",
+ maxWidth: "100%",
+ wordWrap: "break-word",
},
};This change allows inline code blocks to adjust their width based on content while preventing overflow.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const EDITOR_PDF_CODE_STYLES: Styles = { | |
| // code block | |
| "[data-node-type='code-block']": { | |
| marginVertical: convertRemToPixel(0.5), | |
| padding: convertRemToPixel(1), | |
| borderRadius: convertRemToPixel(0.5), | |
| backgroundColor: "#f7f7f7", | |
| fontSize: convertRemToPixel(0.7), | |
| }, | |
| // inline code block | |
| // TODO: update width | |
| "[data-node-type='inline-code-block']": { | |
| margin: 0, | |
| paddingVertical: convertRemToPixel(0.25 / 4 + 0.25 / 8), | |
| paddingHorizontal: convertRemToPixel(0.375), | |
| border: "0.5px solid #e5e5e5", | |
| borderRadius: convertRemToPixel(0.25), | |
| backgroundColor: "#e8e8e8", | |
| color: "#f97316", | |
| fontSize: convertRemToPixel(0.7), | |
| }, | |
| }; | |
| const EDITOR_PDF_CODE_STYLES: Styles = { | |
| // code block | |
| "[data-node-type='code-block']": { | |
| marginVertical: convertRemToPixel(0.5), | |
| padding: convertRemToPixel(1), | |
| borderRadius: convertRemToPixel(0.5), | |
| backgroundColor: "#f7f7f7", | |
| fontSize: convertRemToPixel(0.7), | |
| }, | |
| // inline code block | |
| "[data-node-type='inline-code-block']": { | |
| margin: 0, | |
| paddingVertical: convertRemToPixel(0.25 / 4 + 0.25 / 8), | |
| paddingHorizontal: convertRemToPixel(0.375), | |
| border: "0.5px solid #e5e5e5", | |
| borderRadius: convertRemToPixel(0.25), | |
| backgroundColor: "#e8e8e8", | |
| color: "#f97316", | |
| fontSize: convertRemToPixel(0.7), | |
| display: "inline-block", | |
| maxWidth: "100%", | |
| wordWrap: "break-word", | |
| }, | |
| }; |
| let parsedMarkdownContent = markdownContent; | ||
| // replace the matched mention components with [label](redirect_uri) | ||
| const mentionRegex = /<mention-component[^>]*label="([^"]+)"[^>]*redirect_uri="([^"]+)"[^>]*><\/mention-component>/g; | ||
| const originUrl = window?.location?.origin ?? ""; |
There was a problem hiding this comment.
Ensure window is defined when accessing window.location.origin
At line 137, accessing window.location.origin can result in a ReferenceError if window is undefined, such as in server-side environments.
Consider modifying the code to safely check for window:
-const originUrl = window?.location?.origin ?? "";
+const originUrl = (typeof window !== 'undefined' && window.location.origin) ? window.location.origin : "";This change ensures that the code does not throw an error when window is not available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const originUrl = window?.location?.origin ?? ""; | |
| const originUrl = (typeof window !== 'undefined' && window.location.origin) ? window.location.origin : ""; |
| (_match, label, redirectUri) => `[${label}](${originUrl}/${redirectUri})` | ||
| ); | ||
| // replace the matched image components with <img src={src} > | ||
| const imageComponentRegex = /<image-component[^>]*src="([^"]+)"[^>]*>[^]*<\/image-component>/g; |
There was a problem hiding this comment.
Fix the incorrect regular expression in imageComponentRegex
The regular expression at line 143 includes an empty negated character class [^]*, which matches any character, including line breaks, and may not behave as intended. This can lead to unexpected matches or performance issues.
Apply this diff to correct the regular expression:
-const imageComponentRegex = /<image-component[^>]*src="([^"]+)"[^>]*>[^]*<\/image-component>/g;
+const imageComponentRegex = /<image-component[^>]*src="([^"]+)"[^>]*>.*?<\/image-component>/gs;Explanation:
- Replaced
[^]*with.*?to match any character (non-greedy). - Added the
sflag to allow.to match newline characters, ensuring multiline content is correctly handled.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const imageComponentRegex = /<image-component[^>]*src="([^"]+)"[^>]*>[^]*<\/image-component>/g; | |
| const imageComponentRegex = /<image-component[^>]*src="([^"]+)"[^>]*>.*?<\/image-component>/gs; |
🧰 Tools
🪛 Biome
[error] 143-143: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.(lint/correctness/noEmptyCharacterClassInRegex)
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: "Page could not be exported. Please try again later.", | ||
| }); | ||
| } finally { |
There was a problem hiding this comment.
Log errors for improved debugging
In the handleExport function, errors are caught and a generic error message is displayed to the user. However, the actual error details are not logged, which may hinder debugging efforts. Logging the error can provide valuable insights during development and troubleshooting.
Apply the following change to log the error details:
} catch (error) {
+ console.error('Export error:', error);
setToast({
type: TOAST_TYPE.ERROR,
title: "Error!",
message: "Page could not be exported. Please try again later.",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: "Page could not be exported. Please try again later.", | |
| }); | |
| } finally { | |
| } catch (error) { | |
| console.error('Export error:', error); | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: "Page could not be exported. Please try again later.", | |
| }); | |
| } finally { |
| const selectedPageFormat = watch("page_format"); | ||
| const selectedContentVariety = watch("content_variety"); | ||
| const isPDFSelected = selectedExportFormat === "pdf"; | ||
| const fileName = pageTitle?.toLowerCase()?.replace(/ /g, "-"); |
There was a problem hiding this comment.
Ensure fileName is safe for file system usage
The fileName derived from pageTitle may contain characters that are invalid or unsafe for file names (e.g., /, \, ?, *). This could cause issues when saving the file. Sanitizing fileName will prevent potential errors.
Sanitize fileName to remove or replace unsafe characters:
- const fileName = pageTitle?.toLowerCase()?.replace(/ /g, "-");
+ const fileName = pageTitle
+ ?.toLowerCase()
+ ?.replace(/[^a-z0-9-_]/g, "-")
+ .replace(/-+/g, "-");This will replace any character that is not a lowercase letter, number, hyphen, or underscore with a hyphen and collapse multiple hyphens into one.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fileName = pageTitle?.toLowerCase()?.replace(/ /g, "-"); | |
| const fileName = pageTitle | |
| ?.toLowerCase() | |
| ?.replace(/[^a-z0-9-_]/g, "-") | |
| .replace(/-+/g, "-"); |
| const handleExportAsPDF = async () => { | ||
| try { | ||
| const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`; | ||
| const parsedPageContent = await replaceCustomComponentsFromHTMLContent({ | ||
| htmlContent: pageContent, | ||
| noAssets: selectedContentVariety === "no-assets", | ||
| }); | ||
|
|
||
| const blob = await pdf(<PDFDocument content={parsedPageContent} pageFormat={selectedPageFormat} />).toBlob(); | ||
| const url = URL.createObjectURL(blob); | ||
| const link = document.createElement("a"); | ||
| link.href = url; | ||
| link.download = `${fileName}-${selectedPageFormat.toString().toLowerCase()}.pdf`; | ||
| link.click(); | ||
|
|
||
| URL.revokeObjectURL(url); | ||
| } catch (error) { | ||
| throw new Error(`Error in exporting as a PDF: ${error}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure URL.revokeObjectURL is called after download
In the handleExportAsPDF function, URL.revokeObjectURL(url); is called immediately after link.click();. In some browsers, the download may not start before the URL is revoked.
Consider moving URL.revokeObjectURL(url); into a setTimeout to ensure the download begins:
link.click();
- URL.revokeObjectURL(url);
+ setTimeout(() => {
+ URL.revokeObjectURL(url);
+ }, 1000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleExportAsPDF = async () => { | |
| try { | |
| const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`; | |
| const parsedPageContent = await replaceCustomComponentsFromHTMLContent({ | |
| htmlContent: pageContent, | |
| noAssets: selectedContentVariety === "no-assets", | |
| }); | |
| const blob = await pdf(<PDFDocument content={parsedPageContent} pageFormat={selectedPageFormat} />).toBlob(); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = `${fileName}-${selectedPageFormat.toString().toLowerCase()}.pdf`; | |
| link.click(); | |
| URL.revokeObjectURL(url); | |
| } catch (error) { | |
| throw new Error(`Error in exporting as a PDF: ${error}`); | |
| } | |
| }; | |
| const handleExportAsPDF = async () => { | |
| try { | |
| const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`; | |
| const parsedPageContent = await replaceCustomComponentsFromHTMLContent({ | |
| htmlContent: pageContent, | |
| noAssets: selectedContentVariety === "no-assets", | |
| }); | |
| const blob = await pdf(<PDFDocument content={parsedPageContent} pageFormat={selectedPageFormat} />).toBlob(); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = `${fileName}-${selectedPageFormat.toString().toLowerCase()}.pdf`; | |
| link.click(); | |
| setTimeout(() => { | |
| URL.revokeObjectURL(url); | |
| }, 1000); | |
| } catch (error) { | |
| throw new Error(`Error in exporting as a PDF: ${error}`); | |
| } | |
| }; |
🛠️ Refactor suggestion
Refactor duplicated code in export functions
The handleExportAsPDF and handleExportAsMarkdown functions share similar logic for creating blobs, generating URLs, creating links, and initiating downloads. Refactoring the shared code into a helper function will enhance maintainability and reduce redundancy.
Consider extracting the common logic into a helper function:
+const initiateDownload = (blob: Blob, filename: string) => {
+ const url = URL.createObjectURL(blob);
+ const link = document.createElement("a");
+ link.href = url;
+ link.download = filename;
+ link.click();
+ URL.revokeObjectURL(url);
+};Then update the export functions:
// Inside handleExportAsPDF
- const url = URL.createObjectURL(blob);
- const link = document.createElement("a");
- link.href = url;
- link.download = `${fileName}-${selectedPageFormat.toString().toLowerCase()}.pdf`;
- link.click();
- URL.revokeObjectURL(url);
+ initiateDownload(
+ blob,
+ `${fileName}-${selectedPageFormat.toString().toLowerCase()}.pdf`
+ );Committable suggestion was skipped due to low confidence.
| // handle export as a PDF | ||
| const handleExportAsPDF = async () => { | ||
| try { | ||
| const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`; |
There was a problem hiding this comment.
Sanitize pageTitle to prevent XSS vulnerabilities
The pageTitle is directly inserted into HTML without sanitization, which could lead to Cross-Site Scripting (XSS) vulnerabilities if it contains malicious content. It's important to sanitize or encode pageTitle before embedding it in the HTML.
Consider sanitizing pageTitle:
+ import DOMPurify from 'dompurify';
- const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`;
+ const safePageTitle = DOMPurify.sanitize(pageTitle);
+ const pageContent = `<h1 class="page-title">${safePageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`;Ensure that you have a sanitization library like dompurify installed and imported.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@SatishGandham, I've checked the codebase and found that pageTitle is not currently being sanitized before being embedded into HTML content. This could lead to XSS vulnerabilities if pageTitle contains malicious input.
I recommend sanitizing pageTitle using a library like DOMPurify to prevent potential security risks. Here's how you can modify your code:
import DOMPurify from 'isomorphic-dompurify';
const safePageTitle = DOMPurify.sanitize(pageTitle);
const pageContent = `<h1 class="page-title">${safePageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`;Ensure that isomorphic-dompurify is installed and properly imported in your project. This will sanitize the pageTitle and help prevent potential XSS attacks.
Let me know if you have any questions or need further assistance.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any sanitization applied to 'pageTitle'
rg 'sanitize.*pageTitle'
# Search for any usage of 'pageTitle' where it's sanitized with DOMPurify
rg 'DOMPurify\.sanitize\(\s*pageTitle\s*\)'
# Search for imports of DOMPurify
rg 'import.*DOMPurify'
# Search for all uses of 'pageTitle' to check its usage context
rg 'pageTitle'
Length of output: 12798
| <Controller | ||
| control={control} | ||
| name="export_format" | ||
| render={({ field: { onChange, value } }) => ( | ||
| <CustomSelect | ||
| label={EXPORT_FORMATS.find((format) => format.key === value)?.label} | ||
| buttonClassName="border-none" | ||
| value={value} | ||
| onChange={(val: TExportFormats) => onChange(val)} | ||
| className="flex-shrink-0" | ||
| placement="bottom-end" | ||
| > | ||
| {EXPORT_FORMATS.map((format) => ( | ||
| <CustomSelect.Option key={format.key} value={format.key}> | ||
| {format.label} | ||
| </CustomSelect.Option> | ||
| ))} | ||
| </CustomSelect> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility by adding labels to form controls
The form controls lack explicit labels associated with their respective inputs, which may affect accessibility for screen readers.
Wrap each CustomSelect with a <label> element or use aria-label:
- <h6 className="flex-shrink-0 text-sm text-custom-text-200">Export format</h6>
+ <label className="flex-shrink-0 text-sm text-custom-text-200" htmlFor="export_format">Export format</label>
...
<CustomSelect
+ id="export_format"Repeat this pattern for other form controls.
Committable suggestion was skipped due to low confidence.
| // handle export as a PDF | ||
| const handleExportAsPDF = async () => { | ||
| try { | ||
| const pageContent = `<h1 class="page-title">${pageTitle}</h1>${editorRef?.getDocument().html ?? "<p></p>"}`; |
What's new?
A page can now be exported as a PDF and Markdown file.
Media:
Screen.Recording.2024-09-26.at.14.33.16.mov
Other improvements:
font-familyof all the editors.Plane issue: PE-45
Summary by CodeRabbit
Release Notes
New Features
ExportPageModalfor exporting content in PDF and Markdown formats.PDFDocumentcomponent for rendering PDF documents.SM,MD, andLG.Improvements
Bug Fixes