feat: Ensure the article appears exactly as it will when published #1102 #1153
Conversation
|
@dineshsutihar is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis 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
SlashCommandhas been added to theextensionsarray in theuseEditorhook. 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
RenderPostcomponent setseditable: falsein the editor configuration, yet it includes theSlashCommandextension. This combination seems counterintuitive as slash commands are typically interactive features used in editable contexts.Consider the following:
- If
SlashCommandis intended for read-only rendering of previously entered commands, ensure it's designed to work in non-editable mode.- If
SlashCommandis meant to be interactive, you might need to conditionally include it based on aneditableprop.- Alternatively, if this component is always meant to be read-only, consider removing the
SlashCommandextension 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
SlashCommandsuggests 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
SlashCommandfeature:app/(app)/articles/[slug]/page.tsx (1)
118-119: Verify that the article title is displayed for Tiptap contentCurrently, the article title is only rendered when
isTiptapContentisfalse(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.
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis 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)
|
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 |
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
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 layoutThe 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 renderingThe 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 displayThe 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
savedTimedisplay. 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 likedate-fnsormoment.jsfor this purpose.
260-269: LGTM: Improved publish/save button functionalityThe 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 functionalityThe changes in this file substantially enhance the user interface and experience of the article editor, aligning well with the PR objectives. Key improvements include:
- Better layout structure and responsiveness
- Improved Editor component rendering supporting both JSON and markdown formats
- Enhanced auto-save status display for better user feedback
- 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:
- Consider breaking down this large component into smaller, more manageable sub-components. This would enhance readability and maintainability.
- Implement proper error handling for the auto-save functionality to ensure a smooth user experience even when network issues occur.
- 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
📒 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 componentThe 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.
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 |
|
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: To: |
|
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. |
|
These two lines work perfectly in dark mode, but when you switch to light mode, you might notice that a
What I’ve done for the editor page (pre-publish page):
Changes made for the article page (post-publish page) and details on dependencies:
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 |
|
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. |
|
The following five <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? |
|
@dineshsutihar, thanks for this! Absolutely fantastic notes, and a lot of it makes sense now that I understand it. 🦾 The simpler fix (using the 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). |
|
Eslint and build failure but looks good from a glance. Once the checks pass I'll give a full review. |
dineshsutihar
had a problem deploying
to
production
GitHub Actions
Error
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
|
Fixed all ESLint and Prettier formatting errors. @NiallJoeMaher Can you give full review? |
There was a problem hiding this comment.
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 contentThe
renderSanitizedTiptapContentfunction effectively generates and sanitizes HTML from Tiptap JSON content, addressing potential XSS vulnerabilities.Consider using a more specific type for the
jsonContentparameter: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 supportThe changes in this file effectively implement support for both Markdoc and Tiptap content types, aligning well with the PR objectives. Key improvements include:
- Addition of new utility functions for JSON parsing and Tiptap content rendering.
- Implementation of content type detection and appropriate rendering logic.
- 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:
- Consider extracting the content rendering logic into a separate component or utility function to improve modularity and testability.
- Implement comprehensive unit tests for the new utility functions and rendering logic to ensure robustness across different content types and edge cases.
- 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
📒 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 sanitizationThe 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 functionThe
parseJSONfunction 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 contentThe rendering logic for Tiptap content is implemented correctly. The use of
dangerouslySetInnerHTMLis 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 ofdangerouslySetInnerHTMLis justified in this case because:
- The content is sanitized using DOMPurify before rendering.
- 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 renderingThe 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.
| 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; |
There was a problem hiding this comment.
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.
| 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, | |
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
temporarily deployed
to
production
GitHub Actions
Inactive
|
Just getting prettier issues by the looks of things. Hoping the build will pass 🤞 |
|
I ran |
dineshsutihar
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
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 renderingWhile 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
📒 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 implementationThe
parseJSONutility function properly handles JSON parsing errors and provides good type safety with theJSONContentreturn type.
118-127: Verify title rendering consistency across content typesThe 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)
| const renderSanitizedTiptapContent = (jsonContent: JSONContent) => { | ||
| const rawHtml = generateHTML(jsonContent, [...TiptapExtensions]); | ||
| // Sanitize the HTML | ||
| return DOMPurify.sanitize(rawHtml); | ||
| }; |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| }; |
|
I've fixed the Prettier issues, and the code should be good now. Hopefully, all the tests will pass this time! 🤞 @NiallJoeMaher |
Fixes #1102
Pull Request details
Any Breaking changes
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
[Edited] Current Final status:
2024-10-19.17-20-49.mp4