Add Rich text field and markdown to messages #184
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/__shared__/constants.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThis pull request introduces several enhancements across the components and design system packages, focusing on improving text input, message handling, and validation. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant SocialInput
participant RichTextEditor
participant Sanitizer
participant Backend
User->>SocialInput: Enter message
SocialInput->>RichTextEditor: Capture input
RichTextEditor->>Sanitizer: Sanitize content
Sanitizer-->>RichTextEditor: Cleaned content
RichTextEditor->>SocialInput: Processed text
SocialInput->>Backend: Submit message
Possibly related PRs
Suggested Reviewers
Poem
Finishing Touches
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: 4
🧹 Nitpick comments (12)
packages/design-system/components/inputs/QuillRichTextField/types.ts (1)
8-8: Consider using a more specific type for inputRef.The current type
React.Ref<Quill | null>could be more specific. Since Quill editor has specific methods and properties, consider usingReact.RefObject<Quill>for better type safety and autocomplete support.- inputRef?: React.Ref<Quill | null> + inputRef?: React.RefObject<Quill>packages/components/modules/__shared__/constants.ts (1)
18-19: Consider more specific error message for rich text content.While the validation logic is correct, the error message could be more specific about rich text content being required.
- .refine((value) => value !== '<p><br></p>' && value !== '', 'Must have at least one character'), + .refine( + (value) => value !== '<p><br></p>' && value !== '', + 'Please enter some text content' + ),packages/design-system/components/inputs/SocialTextField/index.tsx (1)
31-31: Good architectural decision making TextField configurable.Making the TextField component configurable through props improves component flexibility and reusability. The default to RichTextEditor with the ability to override is a good pattern.
Also applies to: 53-53
packages/design-system/components/inputs/QuillRichTextField/index.tsx (1)
41-44: Improve type safety for inputRef handling.The current type assertion could be made safer using type guards.
-if (inputRef) { - const mutableRef = inputRef as React.MutableRefObject<Quill | null> - mutableRef.current = quillRef.current -} +if (inputRef && 'current' in inputRef) { + (inputRef as React.MutableRefObject<Quill | null>).current = quillRef.current +}packages/design-system/components/inputs/QuillRichTextField/utils.ts (2)
16-26: Refactor the while loop to improve readability.The current implementation uses assignment in the while condition, which is flagged by static analysis. This can be refactored to be more explicit.
- let match - // eslint-disable-next-line no-cond-assign - while ((match = pattern.exec(lineText))) { + let match = pattern.exec(lineText) + while (match !== null) { const start = match.index const end = start + match[0].length // @ts-ignore quill.deleteText(line.offset(start), end - start) // @ts-ignore quill.insertText(line.offset(start), match[1], format(match)) + match = pattern.exec(lineText) }🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
22-25: Document the reason for type assertions.The
@ts-ignorecomments should include explanations for why they're necessary.- // @ts-ignore + // @ts-ignore: Quill's type definitions are incomplete for the offset method quill.deleteText(line.offset(start), end - start) - // @ts-ignore + // @ts-ignore: Quill's type definitions are incomplete for insertText with formats quill.insertText(line.offset(start), match[1], format(match))packages/components/modules/messages/SendMessage/index.tsx (1)
Line range hint
89-90: Implement error handling for TODO comments.There are multiple TODO comments about error handling that should be addressed.
Would you like me to help implement proper error handling for these cases? I can suggest an error handling implementation that includes:
- User-friendly error messages
- Error logging
- Error recovery strategies
Also applies to: 102-103
packages/components/modules/comments/CommentUpdate/index.tsx (1)
5-5: Consider direct integration with RichTextEditorSince the PR's objective is to add rich text capabilities, consider using the new
RichTextEditorcomponent directly instead ofTextareaField. This would provide markdown support immediately rather than requiring a future update.-import { TextareaField } from '@baseapp-frontend/design-system' +import { RichTextEditor } from '@baseapp-frontend/design-system' // ... SocialTextFieldProps={{ key: comment.id, - TextField: TextareaField, + TextField: RichTextEditor, }}Also applies to: 147-150
packages/components/modules/comments/CommentCreate/index.tsx (1)
6-6: Maintain consistency with CommentUpdate componentFor consistency, both comment components should use the same text field implementation. Consider using
RichTextEditorhere as well to maintain a unified editing experience across the application.-import { TextareaField } from '@baseapp-frontend/design-system' +import { RichTextEditor } from '@baseapp-frontend/design-system' // ... SocialTextFieldProps={{ - TextField: TextareaField, + TextField: RichTextEditor, }}Also applies to: 156-158
packages/design-system/components/inputs/QuillRichTextField/styles.css (2)
14-17: Consider using a lower z-index valueThe z-index of 9999 is extremely high and might cause conflicts with other overlays or modals. Consider using a lower value that still maintains the tooltip's visibility while following your z-index scale.
.ql-bubble .ql-tooltip { @apply bg-grey-900; - z-index: 9999; + z-index: 50; }
8-12: Consider making the max-height configurableThe fixed max-height calculation might not be suitable for all use cases. Consider making it configurable through a CSS variable.
.ql-editor { @apply prose-body2 text-text-primary; - max-height: calc(1.5rem * 8); + max-height: var(--ql-editor-max-height, calc(1.5rem * 8)); overflow-y: auto; }packages/components/modules/messages/utils.ts (1)
31-34: Add type safety and error handling.The implementation could be improved with:
- Type safety for the returned value
- Error handling for parse failures
Consider this implementation:
-export const parseAndSanitize = (html: string) => { +export const parseAndSanitize = (html: string): React.ReactNode => { if (!html) return null - return parse(DOMPurify.sanitize(html, { USE_PROFILES: { html: true } })) + try { + const sanitized = DOMPurify.sanitize(html, { USE_PROFILES: { html: true } }) + return parse(sanitized) + } catch (error) { + console.error('Failed to parse HTML content:', error) + return html // Fallback to plain text + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/CreateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/MembersListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ReadMessagesMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/RoomFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UpdateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/chatRoomsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useRoomListSubscription.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/components/modules/__shared__/constants.ts(1 hunks)packages/components/modules/comments/CommentCreate/index.tsx(2 hunks)packages/components/modules/comments/CommentUpdate/index.tsx(2 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(1 hunks)packages/components/modules/messages/graphql/queries/Room.ts(1 hunks)packages/components/modules/messages/utils.ts(2 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(5 hunks)packages/design-system/components/inputs/QuillRichTextField/index.tsx(1 hunks)packages/design-system/components/inputs/QuillRichTextField/styles.css(1 hunks)packages/design-system/components/inputs/QuillRichTextField/types.ts(1 hunks)packages/design-system/components/inputs/QuillRichTextField/utils.ts(1 hunks)packages/design-system/components/inputs/SocialTextField/index.tsx(3 hunks)packages/design-system/components/inputs/SocialTextField/types.ts(1 hunks)packages/design-system/components/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Main Workflow
packages/design-system/components/inputs/QuillRichTextField/index.tsx
[error] 5-5: CommonJS module imports will produce 'require' calls but the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider using dynamic import instead
packages/design-system/components/inputs/SocialTextField/index.tsx
[error] 14-14: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
🪛 Biome (1.9.4)
packages/design-system/components/inputs/SocialTextField/types.ts
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/design-system/components/inputs/QuillRichTextField/utils.ts
[error] 18-18: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (15)
packages/design-system/components/inputs/QuillRichTextField/types.ts (1)
5-14: LGTM! Well-structured type definition.The type definition is comprehensive and includes all necessary props for a rich text editor component. Good job on extending FormControl for form integration.
packages/design-system/components/inputs/index.ts (1)
13-14: LGTM! Consistent export pattern.The exports follow the established pattern in the codebase, exporting both the component and its types.
packages/design-system/components/inputs/QuillRichTextField/index.tsx (1)
5-5:⚠️ Potential issueUpdate Quill import to use dynamic import for ESM compatibility.
The current import of Quill will produce 'require' calls but the file is an ECMAScript module.
-import Quill from 'quill' +const Quill = await import('quill').then(module => module.default)Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] 5-5: CommonJS module imports will produce 'require' calls but the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider using dynamic import instead
packages/components/modules/messages/SendMessage/index.tsx (1)
127-129: Good use of roomId as key prop.Using roomId as the key prop helps React properly handle component lifecycle when switching between chat rooms.
packages/design-system/package.json (1)
32-32: Verify Quill version compatibility and securityThe Quill dependency is pinned to version 2.0.3. Please verify:
- Compatibility with React 18 and other dependencies
- Any known security vulnerabilities in this version
- Consider using a caret range (^2.0.3) for minor updates
✅ Verification successful
Version 2.0.3 of Quill is appropriate
The pinned version 2.0.3:
- Is the latest available version
- Has no known security vulnerabilities
- Is used in a simple React text editor implementation
- Provides stability for this critical UI component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in quill package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "quill") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check latest version npm view quill versions --json | jq -r '.[-1]'Length of output: 764
Script:
#!/bin/bash # Search for Quill imports and usage rg -l "quill" --type ts --type tsx --type js --type jsx # Search for React components using Quill ast-grep --pattern 'import $_ from "quill"' ast-grep --pattern 'new Quill($$$)'Length of output: 1168
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (2)
23-28: LGTM! Improved text wrapping configuration.The styling changes enhance message display by:
- Using
pre-wrapto preserve whitespace and line breaks- Setting
wordBreakandoverflowWrapfor better handling of long content
9-9: Verify XSS protection in parseAndSanitize.While using DOMPurify is good practice, we should ensure the sanitization is properly configured and tested.
Let's verify the sanitization implementation:
Also applies to: 30-30
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (1)
51-51: LGTM! Using plain text for message preview.Appropriate use of
contentPlainTextfor message previews, which is better suited than rich text content in this context.packages/components/modules/messages/utils.ts (1)
3-4: LGTM! Using robust sanitization libraries.Good choice of libraries:
isomorphic-dompurifyworks in both browser and Node.js environmentshtml-react-parsersafely converts HTML to React elementspackages/components/package.json (1)
37-38: LGTM! Good choice of security-focused dependencies.The addition of
html-react-parserandisomorphic-dompurifyis appropriate for safely handling rich text content. UsingDOMPurifyis particularly important as it helps prevent XSS attacks when rendering user-generated content.packages/components/schema.graphql (5)
181-181: LGTM! Enhanced type safety for participant roles.The change from
StringtoChatRoomParticipantRolesenum improves type safety and validation of participant roles.
205-212: LGTM! Well-defined role enumeration.The
ChatRoomParticipantRolesenum clearly defines the available roles (MEMBER,ADMIN) and provides good documentation.
286-288: LGTM! Sensible defaults for optional fields.Good addition of default values:
deleteImage: false- Safe default for image deletionaddParticipants: []- Empty array for new participantsremoveParticipants: []- Empty array for participant removal
677-677: LGTM! Mutation matches input type changes.The
chatRoomUpdatemutation correctly uses the updatedChatRoomUpdateInputtype.
638-638: Consider accessibility and search implications.The addition of
contentPlainTextis good for accessibility and search functionality. However, ensure that:
- The plain text content is properly generated from rich text
- Special characters and formatting are handled appropriately
Run this script to check for proper plain text handling:
✅ Verification successful
✓ Plain text handling implementation follows best practices
The implementation correctly handles plain text by:
- Converting rich text to plain text on the server side
- Providing both rich (
content) and plain (contentPlainText) versions via GraphQL API- Using plain text version appropriately in UI components for display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for plain text content handling in the codebase # Look for plain text content generation logic rg -A 5 "contentPlainText" --type ts --type tsxLength of output: 75
Script:
#!/bin/bash # Search for contentPlainText implementations and related conversion logic rg "contentPlainText" -A 5 # Look for potential text conversion utilities rg -e "toPlainText|stripHtml|sanitizeText|convertToPlain" -A 3 # Find test files related to content conversion fd "test|spec" -e ts -e js | xargs rg "content.*Text|plain.*text" -iLength of output: 19796
| onCancelReply?: () => void | ||
| replyTargetName?: string | null | ||
| key?: string | ||
| TextField?: FC<TextareaFieldProps> | React.ComponentType<{}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace empty object type with explicit interface.
The current type React.ComponentType<{}> is flagged by static analysis. Consider defining explicit requirements for the component.
- TextField?: FC<TextareaFieldProps> | React.ComponentType<{}>
+ TextField?: FC<TextareaFieldProps> | React.ComponentType<TextareaFieldProps>📝 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.
| TextField?: FC<TextareaFieldProps> | React.ComponentType<{}> | |
| TextField?: FC<TextareaFieldProps> | React.ComponentType<TextareaFieldProps> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| import { Container, OutsideReplyContainer, ReplyContainer } from './styled' | ||
| import { SocialTextFieldProps } from './types' | ||
|
|
||
| const RichTextEditor = dynamic(() => import('../QuillRichTextField'), { ssr: false }) |
There was a problem hiding this comment.
Add file extension to dynamic import path.
The dynamic import is missing a file extension, which is required when using Node.js with --moduleResolution set to 'node16' or 'nodenext'.
-const RichTextEditor = dynamic(() => import('../QuillRichTextField'), { ssr: false })
+const RichTextEditor = dynamic(() => import('../QuillRichTextField/index.tsx'), { ssr: false })📝 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 RichTextEditor = dynamic(() => import('../QuillRichTextField'), { ssr: false }) | |
| const RichTextEditor = dynamic(() => import('../QuillRichTextField/index.tsx'), { ssr: false }) |
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] 14-14: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.
| const rootElement = editorRef.current.querySelector('.ql-editor') | ||
| if (rootElement) { | ||
| rootElement.addEventListener('keydown', (event) => { | ||
| onKeyDown?.(event as unknown as React.KeyboardEvent) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add cleanup for keydown event listener.
The keydown event listener is added but never removed, which could lead to memory leaks.
const rootElement = editorRef.current.querySelector('.ql-editor')
if (rootElement) {
+ const handleKeyDown = (event) => {
+ onKeyDown?.(event as unknown as React.KeyboardEvent)
+ }
- rootElement.addEventListener('keydown', (event) => {
- onKeyDown?.(event as unknown as React.KeyboardEvent)
- })
+ rootElement.addEventListener('keydown', handleKeyDown)
+ return () => {
+ rootElement.removeEventListener('keydown', handleKeyDown)
+ }
}📝 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 rootElement = editorRef.current.querySelector('.ql-editor') | |
| if (rootElement) { | |
| rootElement.addEventListener('keydown', (event) => { | |
| onKeyDown?.(event as unknown as React.KeyboardEvent) | |
| }) | |
| } | |
| const rootElement = editorRef.current.querySelector('.ql-editor') | |
| if (rootElement) { | |
| const handleKeyDown = (event) => { | |
| onKeyDown?.(event as unknown as React.KeyboardEvent) | |
| } | |
| rootElement.addEventListener('keydown', handleKeyDown) | |
| return () => { | |
| rootElement.removeEventListener('keydown', handleKeyDown) | |
| } | |
| } |
There was a problem hiding this comment.
Seems like a good suggestion
| lastMessage { | ||
| id | ||
| content | ||
| contentPlainText |
There was a problem hiding this comment.
💡 Codebase verification
contentPlainText field
The GraphQL schema doesn't include the contentPlainText field in the Message type, but it's being queried in the Room fragment. This mismatch will cause GraphQL validation errors.
- Add
contentPlainText: Stringto the Message type inpackages/components/schema.graphql
🔗 Analysis chain
LGTM! Addition of plain text content field.
The contentPlainText field provides a plain text alternative for message content, which is essential for accessibility, search functionality, and message previews.
Let's verify that the schema supports this field:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GraphQL schema includes contentPlainText field
rg -A 5 'type Message' schema.graphql
Length of output: 94
Script:
#!/bin/bash
# Find GraphQL schema files
fd -e graphql -e gql
# Search for Message type definition in all files
rg -A 5 'type Message' --type ts --type graphql --type gql
Length of output: 150
Script:
#!/bin/bash
# Read schema file and search for Message type and contentPlainText
cat packages/components/schema.graphql | grep -A 10 "type Message"
Length of output: 743
__package_name__package update -v __package_version__Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Dependencies
html-react-parserandisomorphic-dompurifyquillrich text editorPerformance