Skip to content
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

feat: Ensure the article appears exactly as it will when published #1102 #1153

Conversation

dineshsutihar
Copy link
Contributor

@dineshsutihar dineshsutihar commented Oct 18, 2024

Fixes #1102

Pull Request details

  • Implement exact rendering of articles on the article page, ensuring they appear as the user typed.
  • This PR supports backward compatibility with previous articles stored in markdown format, while properly rendering new articles stored in the JSON format.
  • Implement DOMPurify to sanitize HTML and protect against XSS attacks.

Any Breaking changes

  • If the "Excerpt" input is left blank in the alpha editor, the default preview will display JSON output. This happens because the article content is stored in JSON format, and without an excerpt, the backend defaults to using the article content for the preview.

Associated Videos

Preview of new JSON content:

2024-10-19.00-10-28.mp4

Preview of old Markdown content:

2024-10-19.00-12-07.mp4

Screenshot of issue

image

[Edited] Current Final status:

2024-10-19.17-20-49.mp4

Copy link

vercel bot commented Oct 18, 2024

@dineshsutihar is attempting to deploy a commit to the Codú Team on Vercel.

A member of the Team first needs to authorize it.

@dineshsutihar dineshsutihar marked this pull request as ready for review October 18, 2024 20:10
@dineshsutihar dineshsutihar requested a review from a team as a code owner October 18, 2024 20:10
Copy link
Contributor

coderabbitai bot commented Oct 18, 2024

Walkthrough

The pull request introduces significant updates to the ArticlePage and RenderPost components, enhancing their functionality to support multiple content types. The ArticlePage now integrates Tiptap alongside Markdoc for rendering, with new utility functions added to handle Tiptap JSON content. The RenderPost component has been updated to include a SlashCommand feature, while the TiptapExtensions have been modified to remove the SlashCommand extension. These changes collectively improve the editor's WYSIWYG capabilities.

Changes

File Path Change Summary
app/(app)/articles/[slug]/page.tsx Updated ArticlePage to handle Tiptap and Markdoc content with new utility functions added.
components/editor/editor/RenderPost.tsx Enhanced RenderPost to integrate SlashCommand feature and modified useEditor configuration.
components/editor/editor/extensions/index.tsx Removed SlashCommand from TiptapExtensions and retained configurations for other extensions.
app/(app)/alpha/new/[[...postIdArr]]/_client.tsx Adjusted layout and structure in Create component, removed unused RenderPost import, and refined form submission logic.

Assessment against linked issues

Objective Addressed Explanation
Support WYSIWYG functionality in the editor (#1102)
Remove the preview step in the editing process (#1102)

Possibly related PRs

Suggested labels

hacktoberfest-accepted, hacktoberfest

Suggested reviewers

  • NiallJoeMaher

🐰 "In the meadow where the edits flow,
Tiptap and Markdoc put on a show.
With commands that dash and content that gleams,
Our WYSIWYG dreams are more than just dreams!
Hop along, let's celebrate this change,
For every article, a new range!" 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dineshsutihar dineshsutihar marked this pull request as draft October 18, 2024 20:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 474c544 and 42cd56d.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json, !**/*.json
  • package.json is excluded by !**/*.json
📒 Files selected for processing (3)
  • app/(app)/articles/[slug]/page.tsx (5 hunks)
  • components/editor/editor/RenderPost.tsx (2 hunks)
  • components/editor/editor/extensions/index.tsx (0 hunks)
💤 Files with no reviewable changes (1)
  • components/editor/editor/extensions/index.tsx
🧰 Additional context used
🪛 Biome
app/(app)/articles/[slug]/page.tsx

[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)

🔇 Additional comments (4)
components/editor/editor/RenderPost.tsx (3)

15-15: SlashCommand integrated into the editor extensions.

The SlashCommand has been added to the extensions array in the useEditor hook. This change effectively integrates the new feature into the editor setup.

Let's verify if this change is consistent across the codebase:

#!/bin/bash
# Description: Verify the consistent usage of SlashCommand in editor configurations

# Test: Check for other instances where editor extensions are configured
rg --type typescript "extensions.*TiptapExtensions" -A 5 -g '!components/editor/editor/RenderPost.tsx'

Line range hint 11-20: Reconsider adding SlashCommand to a non-editable editor.

The RenderPost component sets editable: false in the editor configuration, yet it includes the SlashCommand extension. This combination seems counterintuitive as slash commands are typically interactive features used in editable contexts.

Consider the following:

  1. If SlashCommand is intended for read-only rendering of previously entered commands, ensure it's designed to work in non-editable mode.
  2. If SlashCommand is meant to be interactive, you might need to conditionally include it based on an editable prop.
  3. Alternatively, if this component is always meant to be read-only, consider removing the SlashCommand extension to avoid potential conflicts or unnecessary overhead.

Let's verify the intended behavior of SlashCommand in non-editable contexts:

#!/bin/bash
# Description: Check SlashCommand behavior in non-editable contexts

# Test: Look for any special handling of non-editable mode in SlashCommand
ast-grep --lang typescript --pattern 'function SlashCommand() {
  $$$
  return {
    $$$
    editable: $EDITABLE,
    $$$
  }
}'

4-4: New SlashCommand feature added to the editor.

The import of SlashCommand suggests a new feature being integrated into the editor. This aligns with the PR objective of implementing a more intuitive editing experience.

Let's verify the implementation of the SlashCommand feature:

app/(app)/articles/[slug]/page.tsx (1)

118-119: Verify that the article title is displayed for Tiptap content

Currently, the article title is only rendered when isTiptapContent is false (Markdoc content). For Tiptap content, the title may not be displayed unless it's included within the content. To maintain a consistent user experience, please verify that the title is displayed appropriately for Tiptap content.

app/(app)/articles/[slug]/page.tsx Outdated Show resolved Hide resolved
app/(app)/articles/[slug]/page.tsx Show resolved Hide resolved
@dineshsutihar dineshsutihar marked this pull request as ready for review October 18, 2024 21:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 42cd56d and 753bc7f.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json, !**/*.json
  • package.json is excluded by !**/*.json
📒 Files selected for processing (1)
  • app/(app)/articles/[slug]/page.tsx (5 hunks)
🧰 Additional context used
🪛 Biome
app/(app)/articles/[slug]/page.tsx

[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)

app/(app)/articles/[slug]/page.tsx Show resolved Hide resolved
app/(app)/articles/[slug]/page.tsx Outdated Show resolved Hide resolved
@NiallJoeMaher
Copy link
Contributor

Based on the title "Ensure the article appears exactly as it will when published" I had different expectations.

The problem is it still looks very different pre publish than in the finished product.

Something more like this

Screen.Recording.2024-10-19.at.09.03.36.mov

@dineshsutihar
Copy link
Contributor Author

Based on the title "Ensure the article appears exactly as it will when published" I had different expectations.

The problem is it still looks very different pre publish than in the finished product.

Something more like this

Screen.Recording.2024-10-19.at.09.03.36.mov

Got it, I'm working on the pre-publish editor design. Could you also review the post-publish video I shared and let me know if there are any issues or improvements needed? @NiallJoeMaher

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
app/(app)/alpha/new/[[...postIdArr]]/_client.tsx (5)

216-224: LGTM: Improved main content area layout

The introduction of new wrapper divs and the adjusted layout structure enhance the styling and responsiveness of the main content area. This aligns well with the PR's objective of improving the editor's appearance.

Consider adding comments to explain the purpose of each wrapper div, especially for the ones with specific class names like lg:min-w-0 lg:flex-1. This will improve code maintainability for future developers.


225-242: LGTM: Improved Editor component rendering

The conditional rendering of the Editor component ensures proper initialization for both new and existing content. This change supports the transition to storing articles in JSON format while maintaining backward compatibility with markdown, aligning with the PR objectives.

Consider extracting the Editor rendering logic into a separate component or function to improve readability and maintainability. This would also make it easier to add any future enhancements to the Editor initialization process.


244-258: LGTM: Enhanced auto-save status display

The addition of auto-save status messages significantly improves user feedback. Displaying the last saved time helps users understand the current state of their work, enhancing the overall user experience. This aligns well with the PR's objective of improving the editing experience.

Consider using a more user-friendly time format for the savedTime display. Instead of showing the full timestamp, you could use a relative time format (e.g., "2 minutes ago") for better readability. You can use libraries like date-fns or moment.js for this purpose.


260-269: LGTM: Improved publish/save button functionality

The conditional button text and disabled state improve clarity and prevent unintended actions. The styling is consistent with the overall design, enhancing the user interface.

Consider changing the button text to "Publish..." or "Save Changes..." (with ellipsis) to indicate that clicking the button will open a dialog rather than immediately performing the action. This small change can improve user expectations and align with common UI patterns.


Line range hint 1-285: Overall LGTM: Significant improvements to the article editor UI and functionality

The changes in this file substantially enhance the user interface and experience of the article editor, aligning well with the PR objectives. Key improvements include:

  1. Better layout structure and responsiveness
  2. Improved Editor component rendering supporting both JSON and markdown formats
  3. Enhanced auto-save status display for better user feedback
  4. Refined publish/save button functionality

These changes contribute to a more intuitive editing experience while maintaining backward compatibility with previously stored articles.

To further improve the component:

  1. Consider breaking down this large component into smaller, more manageable sub-components. This would enhance readability and maintainability.
  2. Implement proper error handling for the auto-save functionality to ensure a smooth user experience even when network issues occur.
  3. Add unit tests for the new functionality, especially around the conditional rendering of the Editor component and the auto-save status display.

These suggestions will help ensure the long-term maintainability and reliability of the article editor.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 753bc7f and 37a91d2.

📒 Files selected for processing (1)
  • app/(app)/alpha/new/[[...postIdArr]]/_client.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/(app)/alpha/new/[[...postIdArr]]/_client.tsx (1)

140-140: LGTM: Improved icon rotation in Disclosure component

The class name correction for the ChevronUpIcon ensures proper rotation behavior based on the disclosure state. This change enhances the user interface, aligning with the PR's objective of improving the editing experience.

@dineshsutihar
Copy link
Contributor Author

Based on the title "Ensure the article appears exactly as it will when published" I had different expectations.

The problem is it still looks very different pre publish than in the finished product.

Something more like this

Screen.Recording.2024-10-19.at.09.03.36.mov

Hey @NiallJoeMaher , I've updated the pre-publish editor design to exactly match the article view based on your feedback. I've also attached the video showing the final result. Please review and let me know if there are any further issues or improvements you'd like to see.

2024-10-19.17-20-49.mp4

@NiallJoeMaher
Copy link
Contributor

Hey, thanks for this. But this is a mountain of unneeded code and dependencies.

We can solve this with just 2 lines of code.

I was able to fix this one by updating just the 2 parent wrappers of the controller from:

<div className="bg-neutral-900 text-white shadow-xl">
    <div className="bg-neutral-900 px-4 py-6 sm:p-6 lg:pb-8">

To:

<div className="mx-auto break-words px-2 pb-4 sm:px-4 md:max-w-3xl">
  <div className="prose mx-auto max-w-3xl dark:prose-invert lg:prose-lg">
    {!body && (
      <Controller
        name="body"
        control={control}
        render={({ field }) => (
          <Editor {...field} initialValue={"{}"} />
        )}
      />
    )}
...

Copy link

Uh oh! @dineshsutihar, the image you shared is missing helpful alt text. Check #1153 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@dineshsutihar
Copy link
Contributor Author

These two lines work perfectly in dark mode, but when you switch to light mode, you might notice that a div with className="bg-black" is causing issues, as shown below :

image

What I’ve done for the editor page (pre-publish page):

  • I added the same two lines as yours.
    I removed an unnecessary div with className="bg-black", which was causing the issue. Due to this removal, the code inside was shifted one tab to the left, making it look like there were more changes than there actually were. In reality, it's just the addition of two lines and the removal of one.
  • Removed unnecessary imports to streamline the code (1 removed).

Changes made for the article page (post-publish page) and details on dependencies:

  • Add two packages: @tiptap/html and isomorphic-dompurify

    • @tiptap/html: Recommended by Tiptap for parsing the JSON content (generated by the editor) into HTML on the server-side.
    • isomorphic-dompurify: Used to sanitize the parsed HTML before injecting it into the DOM. This is crucial to mitigate the risk of XSS (Cross-Site Scripting) attacks, ensuring that no malicious scripts are executed when displaying user-generated content.
  • Modified components/editor/editor/extensions/index.tsx:

    • Removes the SlashCommand extension from this file because it was not being configured and it was also a client-side component that caused issues when trying to use it in a server-side context. Now we can use Extensions anywere in application.
  • Changes in app/(app)/articles/[slug]/page.tsx:

    • Database contains articles in both Markdown (old data) and JSON (current data), I updated the code to handle both formats. It now checks if the data is valid JSON; if so, it uses the new parsing method, otherwise, it falls back to the old Markdown parsing using @markdoc/markdoc.
    • Sanitized the parsed HTML with isomorphic-dompurify.
  • Reason for changes:

    • There was no method for parsing JSON, which caused the articles to display incorrectly, as shown below:
      image

Is there anything else you'd like me to adjust, or does this solution work for you? Let me know if you have any questions or concerns! @NiallJoeMaher

Copy link

Uh oh! @dineshsutihar, the image you shared is missing helpful alt text. Check #1153 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@dineshsutihar
Copy link
Contributor Author

The following five divs elements can also be removed:

<div className="mx-auto w-full max-w-7xl flex-grow text-black lg:flex xl:px-8">
  {/* Left sidebar & main wrapper */}
  <div className="min-w-0 flex-1 xl:flex">
    <div className="lg:min-w-0 lg:flex-1">
      <div className="h-full min-h-[40rem] px-4 py-0 sm:px-6 lg:px-4">
        {/* Start main area */}
        <div className="relative h-full">
          {/* Main editor code that is unaffected by the above changes */}
        </div>
        {/* End main area */}
      </div>
    </div>
  </div>
</div>

What do you think of this? Should I remove it as well?

@NiallJoeMaher
Copy link
Contributor

@dineshsutihar, thanks for this! Absolutely fantastic notes, and a lot of it makes sense now that I understand it. 🦾

The simpler fix (using the .prose class) for the moment will ensure the two stay in sync. Old and new editors (thanks for pointing out the light/issue.)

We have an issue open to fix the slash command (#1130). I think it was accidentally removed in a recent PR. It's an important feature, so keeping it for now will make that issue easier (or one you can tackle after this). Or if this one fixes this issue link it in the PR too! (You can split it if you want 2 Hacktoberfest points).

I like how you manage the two different previews (for markdown or JSON). And makes sense on the two packages you have added for this now that you have explained it. Cheers 🍻

I see the excerpt issue. It's not a problem for the minute because we haven't updated the backend. The plan for saving the data is to save the raw text (for search) without JSON. The excerpt should be raw text (no JSON or markdown). We can make an issue if you want to continue work on it.

In the future, we will have a markdown column, a JSON column, and a raw text column, which should make it easier for the logic (for choosing which parsing tool). I will write a script to migrate all old content to JSON so it's consistent too.

If this PR tackles some side issues, let's open up some separate issues, and I can assign them to you, too (the smaller the PR, the easier it is to review).

Copy link

Uh oh! @dineshsutihar, the image you shared is missing helpful alt text. Check #1153 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
codu ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 7:01am

@NiallJoeMaher
Copy link
Contributor

Eslint and build failure but looks good from a glance. Once the checks pass I'll give a full review.

@dineshsutihar
Copy link
Contributor Author

Fixed all ESLint and Prettier formatting errors.

@NiallJoeMaher Can you give full review?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
app/(app)/articles/[slug]/page.tsx (2)

73-77: LGTM: Secure rendering of Tiptap content

The renderSanitizedTiptapContent function effectively generates and sanitizes HTML from Tiptap JSON content, addressing potential XSS vulnerabilities.

Consider using a more specific type for the jsonContent parameter:

const renderSanitizedTiptapContent = (jsonContent: JSONContent) => {
  // ... (rest of the function remains the same)
};

This change improves type safety by ensuring that only valid Tiptap JSON content is passed to the function.


Line range hint 1-158: Summary: Successful implementation of multi-content type support

The changes in this file effectively implement support for both Markdoc and Tiptap content types, aligning well with the PR objectives. Key improvements include:

  1. Addition of new utility functions for JSON parsing and Tiptap content rendering.
  2. Implementation of content type detection and appropriate rendering logic.
  3. Secure handling of user-generated content through sanitization.

These changes contribute to the goal of ensuring articles appear exactly as they will when published, while maintaining backward compatibility with previously stored articles.

To further improve the implementation:

  1. Consider extracting the content rendering logic into a separate component or utility function to improve modularity and testability.
  2. Implement comprehensive unit tests for the new utility functions and rendering logic to ensure robustness across different content types and edge cases.
  3. Document the expected structure of Tiptap content, especially regarding the title rendering, to maintain consistency across the application.

These suggestions will enhance the maintainability and reliability of the multi-content type support in the long term.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 516e714 and c548efd.

📒 Files selected for processing (1)
  • app/(app)/articles/[slug]/page.tsx (5 hunks)
🧰 Additional context used
🪛 Biome
app/(app)/articles/[slug]/page.tsx

[error] 123-123: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)

🔇 Additional comments (4)
app/(app)/articles/[slug]/page.tsx (4)

2-2: LGTM: New imports support Tiptap integration and HTML sanitization

The added imports for Tiptap, DOMPurify, and JSONContent type are appropriate for the new functionality. The RenderableTreeNode import from @markdoc/markdoc aligns with the existing Markdoc usage.

Also applies to: 17-20


65-71: LGTM: Optimized JSON parsing function

The parseJSON function efficiently handles JSON parsing and error cases. This implementation addresses the previous review comment about optimizing JSON parsing.


121-128: LGTM: Secure rendering of Tiptap content

The rendering logic for Tiptap content is implemented correctly. The use of dangerouslySetInnerHTML is necessary for rendering HTML content, and the security risk is mitigated by sanitizing the content before rendering.

Regarding the static analysis hint about dangerouslySetInnerHTML:
The use of dangerouslySetInnerHTML is justified in this case because:

  1. The content is sanitized using DOMPurify before rendering.
  2. It's necessary for rendering the HTML content generated by Tiptap.

The "tiptap-content" class allows for specific styling of Tiptap content, which is a good practice for maintaining consistent appearance.

🧰 Tools
🪛 Biome

[error] 123-123: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)


119-119: Verify Tiptap content structure for title rendering

The conditional rendering of the article title assumes that Tiptap content includes the title within its structure. This may lead to inconsistent rendering between Tiptap and Markdoc content.

Please confirm that all Tiptap content includes the article title. If not, consider rendering the title consistently for both content types:

<h1>{post.title}</h1>
{isTiptapContent ? (
  // Tiptap content rendering
) : (
  // Markdoc content rendering
)}

To verify the Tiptap content structure, you can run the following script:

This script will help identify if Tiptap content consistently includes a title element at the beginning of the document.

Comment on lines +91 to +104
const parsedBody = parseJSON(post.body);
const isTiptapContent = parsedBody?.type === "doc";

let renderedContent: string | RenderableTreeNode;

if (isTiptapContent && parsedBody) {
const jsonContent = parsedBody;
renderedContent = renderSanitizedTiptapContent(jsonContent);
} else {
const ast = Markdoc.parse(post.body);
const transformedContent = Markdoc.transform(ast, config);
renderedContent = Markdoc.renderers.react(transformedContent, React, {
components: markdocComponents,
}) as unknown as string;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

LGTM: Improved content type detection and rendering

The new logic effectively detects and renders both Tiptap and Markdoc content, aligning with the PR objectives.

Remove the unnecessary type assertion in the Markdoc rendering path:

- renderedContent = Markdoc.renderers.react(transformedContent, React, {
-   components: markdocComponents,
- }) as unknown as string;
+ renderedContent = Markdoc.renderers.react(transformedContent, React, {
+   components: markdocComponents,
+ });

This change eliminates the incorrect casting of a React element to string, improving type safety.

📝 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.

Suggested change
const parsedBody = parseJSON(post.body);
const isTiptapContent = parsedBody?.type === "doc";
let renderedContent: string | RenderableTreeNode;
if (isTiptapContent && parsedBody) {
const jsonContent = parsedBody;
renderedContent = renderSanitizedTiptapContent(jsonContent);
} else {
const ast = Markdoc.parse(post.body);
const transformedContent = Markdoc.transform(ast, config);
renderedContent = Markdoc.renderers.react(transformedContent, React, {
components: markdocComponents,
}) as unknown as string;
const parsedBody = parseJSON(post.body);
const isTiptapContent = parsedBody?.type === "doc";
let renderedContent: string | RenderableTreeNode;
if (isTiptapContent && parsedBody) {
const jsonContent = parsedBody;
renderedContent = renderSanitizedTiptapContent(jsonContent);
} else {
const ast = Markdoc.parse(post.body);
const transformedContent = Markdoc.transform(ast, config);
renderedContent = Markdoc.renderers.react(transformedContent, React, {
components: markdocComponents,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the type assertion because renderedContent needs to be treated as a string for use in dangerouslySetInnerHTML, and removing it could lead to runtime issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dineshsutihar, understood. Thanks for the clarification. Keeping the type assertion ensures correct behavior when using dangerouslySetInnerHTML.

🐰🤔


✏️ Learnings added
Learnt from: dineshsutihar
PR: codu-code/codu#1153
File: app/(app)/articles/[slug]/page.tsx:91-104
Timestamp: 2024-10-22T08:43:13.045Z
Learning: In `app/(app)/articles/[slug]/page.tsx`, when rendering content, the `renderedContent` needs to be treated as a string using a type assertion because it's used with `dangerouslySetInnerHTML`, and removing the type assertion could lead to runtime issues.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

NiallJoeMaher
NiallJoeMaher previously approved these changes Oct 22, 2024
Copy link
Contributor

@NiallJoeMaher NiallJoeMaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Excited to get this merged. 🌮

@NiallJoeMaher
Copy link
Contributor

Just getting prettier issues by the looks of things. Hoping the build will pass 🤞

@dineshsutihar
Copy link
Contributor Author

I ran ESLint and Prettier commands multiple times before pushing the code and didn't encounter any Prettier errors. Let me install Prettier globally to see if that fixes the issue.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
app/(app)/articles/[slug]/page.tsx (1)

91-105: Consider adding error boundaries for content rendering

While the code handles JSON parsing errors, it would be beneficial to add error boundaries to gracefully handle rendering failures for both content types.

Consider implementing a custom error boundary component:

class ContentErrorBoundary extends React.Component {
  state = { hasError: false };
  
  static getDerivedStateFromError(error: Error) {
    return { hasError: true };
  }
  
  render() {
    if (this.state.hasError) {
      return <div>Error rendering article content. Please try again later.</div>;
    }
    return this.props.children;
  }
}

This would help maintain a good user experience even if rendering fails.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c548efd and 0a49461.

📒 Files selected for processing (2)
  • app/(app)/alpha/new/[[...postIdArr]]/_client.tsx (1 hunks)
  • app/(app)/articles/[slug]/page.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/(app)/alpha/new/[[...postIdArr]]/_client.tsx
🧰 Additional context used
📓 Learnings (1)
app/(app)/articles/[slug]/page.tsx (1)
Learnt from: dineshsutihar
PR: codu-code/codu#1153
File: app/(app)/articles/[slug]/page.tsx:91-104
Timestamp: 2024-10-22T08:43:13.236Z
Learning: In `app/(app)/articles/[slug]/page.tsx`, when rendering content, the `renderedContent` needs to be treated as a string using a type assertion because it's used with `dangerouslySetInnerHTML`, and removing the type assertion could lead to runtime issues.
🪛 Biome
app/(app)/articles/[slug]/page.tsx

[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)

🔇 Additional comments (2)
app/(app)/articles/[slug]/page.tsx (2)

65-71: LGTM: Robust JSON parsing implementation

The parseJSON utility function properly handles JSON parsing errors and provides good type safety with the JSONContent return type.


118-127: Verify title rendering consistency across content types

The title is only rendered for non-Tiptap content (!isTiptapContent). This might lead to inconsistent article appearances.

Let's verify if Tiptap content includes the title within its JSON structure:

If Tiptap content doesn't inherently include the title, consider standardizing the title rendering:

 <article className="prose mx-auto max-w-3xl dark:prose-invert lg:prose-lg">
-  {!isTiptapContent && <h1>{post.title}</h1>}
+  <h1>{post.title}</h1>
🧰 Tools
🪛 Biome

[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.

Setting content using code can expose users to cross-site scripting (XSS) attacks

(lint/security/noDangerouslySetInnerHtml)

Comment on lines +73 to +77
const renderSanitizedTiptapContent = (jsonContent: JSONContent) => {
const rawHtml = generateHTML(jsonContent, [...TiptapExtensions]);
// Sanitize the HTML
return DOMPurify.sanitize(rawHtml);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider memoizing the TiptapExtensions array

The renderSanitizedTiptapContent function creates a new array spread of TiptapExtensions on every call, which could impact performance for large articles.

+const tiptapExtensions = [...TiptapExtensions];
+
 const renderSanitizedTiptapContent = (jsonContent: JSONContent) => {
-  const rawHtml = generateHTML(jsonContent, [...TiptapExtensions]);
+  const rawHtml = generateHTML(jsonContent, tiptapExtensions);
   // Sanitize the HTML
   return DOMPurify.sanitize(rawHtml);
 };
📝 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.

Suggested change
const renderSanitizedTiptapContent = (jsonContent: JSONContent) => {
const rawHtml = generateHTML(jsonContent, [...TiptapExtensions]);
// Sanitize the HTML
return DOMPurify.sanitize(rawHtml);
};
const tiptapExtensions = [...TiptapExtensions];
const renderSanitizedTiptapContent = (jsonContent: JSONContent) => {
const rawHtml = generateHTML(jsonContent, tiptapExtensions);
// Sanitize the HTML
return DOMPurify.sanitize(rawHtml);
};

@dineshsutihar
Copy link
Contributor Author

I've fixed the Prettier issues, and the code should be good now. Hopefully, all the tests will pass this time! 🤞 @NiallJoeMaher

Copy link
Contributor

@NiallJoeMaher NiallJoeMaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮 🦖 Nice one!

@NiallJoeMaher NiallJoeMaher merged commit 732d91f into codu-code:develop Oct 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement WYSIWYG for the alpha editor.
2 participants