variables and mentions #590
Conversation
WalkthroughThe pull request introduces multiple changes across various classes and components. Key modifications include updates to validation rules in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 25
🧹 Outside diff range and nitpick comments (16)
api/app/Service/HtmlPurifier/OpenFormsHtmlDefinition.php (1)
11-20: LGTM: Theapplymethod is well-implemented with a minor suggestion.The method correctly extends the
Html5Definitionand adds custom attributes for mentions and content editability. The attribute types are properly specified.Consider grouping related attributes together for improved readability:
public static function apply(HTMLPurifier_HTMLDefinition $definition) { Html5Definition::apply($definition); $definition->addAttribute('span', 'mention-field-id', 'Text'); $definition->addAttribute('span', 'mention-field-name', 'Text'); $definition->addAttribute('span', 'mention-fallback', 'Text'); $definition->addAttribute('span', 'mention', 'Bool'); + $definition->addAttribute('span', 'contenteditable', 'Bool'); }client/components/open/integrations/SubmissionConfirmationIntegration.vue (3)
30-37: Enhanced input components with mention functionality.The replacement of
text-inputwithMentionInputfor subject and reply-to fields, and the addition of mention support to the email content field, greatly improve the component's functionality. This allows for dynamic content insertion from form properties, enhancing email customization capabilities.Consider adding a brief help text or tooltip to explain the mention functionality to users who might be unfamiliar with it.
Also applies to: 41-42, 47-53
Line range hint
105-107: Improved initialization of respondent_email setting.The modification to the
onBeforeMounthook ensures that therespondent_emailsetting is only initialized when an email field is present in the form. This change prevents potential issues with email confirmation when no email field is available.Consider adding a comment explaining the purpose of this conditional initialization to improve code readability and maintainability.
Line range hint
1-107: Consider enhancing error handling for email confirmation.While not directly related to the current changes, it would be beneficial to implement or improve error handling for cases where email sending might fail. This could include displaying user-friendly error messages and potentially implementing a retry mechanism.
Consider adding a method to handle email sending errors, for example:
const handleEmailError = (error) => { // Log the error console.error('Email sending failed:', error); // Display a user-friendly message // (assuming you have a notification system in place) showNotification({ type: 'error', message: 'Failed to send confirmation email. Please try again later.' }); // Optionally, implement a retry mechanism // retryEmailSending(); }This method could be called in the appropriate place where email sending is initiated.
api/app/Integrations/Handlers/SubmissionConfirmationIntegration.php (1)
110-112: Approve the addition of purification for 'notification_subject'.The addition of purification for the 'notification_subject' field enhances security by sanitizing the input. This is consistent with the existing purification of 'notification_body'.
For consistency, consider updating the 'notification_body' line to use the null coalescing operator (??) as well:
return array_merge(parent::formatData($data), [ 'notification_subject' => Purify::clean($data['notification_subject'] ?? ''), - 'notification_body' => Purify::clean($data['notification_body'] ?? ''), + 'notification_body' => Purify::clean($data['notification_body'] ?? ''), ]);This change ensures that both fields are handled identically.
client/components/forms/TextBlock.vue (1)
35-35: Removeconsole.tracestatement from production code.The
console.tracestatement on line 35 appears to be used for debugging purposes. It's best practice to remove such statements from production code to avoid unnecessary console output and potential performance overhead.client/lib/quill/quillMentionExtension.js (2)
24-24: Consider using the class name instead ofsuperin static methodsUsing
superin a static context can be unclear to some developers. For improved readability, consider using the class nameEmbeddirectly when calling the parent class's static method.Apply this diff to enhance clarity:
- const node = super.create() + const node = Embed.create()🧰 Tools
🪛 Biome
[error] 24-24: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
39-39: Extract the truncation length into a constantThe magic number
25is used to truncate thefield.name. For better maintainability and readability, consider extracting it into a named constant.Apply this diff to define a constant for the maximum name length:
+ const MAX_NAME_LENGTH = 25; //... - node.textContent = data.field.name.length > 25 ? `${data.field.name.slice(0, 25)}...` : data.field.name + node.textContent = data.field.name.length > MAX_NAME_LENGTH ? `${data.field.name.slice(0, MAX_NAME_LENGTH)}...` : data.field.nameapi/app/Mail/Forms/SubmissionConfirmationMail.php (2)
85-89: Ensure Unit Tests Cover the NewgetSubject()MethodThe
getSubject()method is critical for generating dynamic email subjects using theMentionParser. To maintain reliability, please ensure that unit tests are added or updated to cover various input scenarios, including edge cases.
79-79: Validate Email with Laravel's Validation RulesCurrently, email validation uses
filter_var()withFILTER_VALIDATE_EMAIL. Consider using Laravel's built-inValidatorfor more comprehensive email validation.Example:
use Illuminate\Support\Facades\Validator; if ($parsedReplyTo) { $validator = Validator::make(['email' => $parsedReplyTo], ['email' => 'email']); if ($validator->passes()) { return $parsedReplyTo; } }client/components/forms/components/MentionDropdown.vue (2)
15-19: Add a label to theFallback valueinput for accessibilityTo improve accessibility, associate a label with the
Fallback valueinput. This helps screen readers and improves form usability.Example:
<label class="sr-only" for="fallback-input">Fallback value</label> <input id="fallback-input" v-model="fallbackValue" class="p-1 mb-2 text-sm w-1/2 border rounded-md hover:bg-gray-50" placeholder="Fallback value" />
13-14: Externalize hardcoded strings for localizationTo support internationalization, consider moving hardcoded strings like
"Insert Mention","Select a field","Insert", and"Cancel"to a localization file.This allows for easier translation and adapts the application for a wider audience.
Also applies to: 23-25, 55-56, 62-63
client/components/forms/RichTextAreaInput.client.vue (1)
27-31: Clarify help text for better user understandingConsider updating the help text to be more descriptive. For example, "Type @ to mention form variables" might be clearer to the user.
Apply this diff to update the help text:
-help="Click @ to use form variables" +help="Type @ to mention form variables"client/components/forms/MentionInput.vue (1)
81-90: Consistently handle mention field name truncationIn
createMentionSpan, the mention's field name is truncated if it exceeds 25 characters. To ensure consistency and maintainability, consider handling truncation with CSS rather than JavaScript.Modify the JavaScript to use the full
mention.field.name:-mentionSpan.textContent = mention.field.name.length > 25 ? `${mention.field.name.slice(0, 25)}...` : mention.field.name +mentionSpan.textContent = mention.field.nameUpdate the CSS to handle text overflow:
.mention-input span[mention] { /* existing styles */ max-width: 150px; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; }client/components/open/forms/components/form-components/FormSubmissionSettings.vue (1)
Line range hint
70-90: Inconsistent usage ofProTagcomponent in the templateThe
ProTagcomponent is used inconsistently within the template. It is imported asProTagbut used both as<pro-tag>and<ProTag>. For consistency and to prevent potential issues, it's recommended to use the same casing throughout the template. Since the component is imported in PascalCase, consider updating all instances to<ProTag>.Apply the following diff to maintain consistency:
-<pro-tag +<ProTag v-if="option === 'redirect'" class="ml-2" />client/components/open/forms/OpenCompleteForm.vue (1)
229-230: InitializesubmissionIdtonullinstead offalsefor clarityInitializing
submissionIdtonullrather thanfalseimproves code clarity, asnullmore accurately represents the absence of a value, whereasfalseis a boolean value.Apply this diff to update the initialization:
data () { return { loading: false, submitted: false, passwordForm: useForm({ password: null }), hidePasswordDisabledMsg: false, - submissionId: false, + submissionId: null, submittedData: null } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- api/app/Integrations/Handlers/SubmissionConfirmationIntegration.php (2 hunks)
- api/app/Mail/Forms/SubmissionConfirmationMail.php (4 hunks)
- api/app/Open/MentionParser.php (1 hunks)
- api/app/Service/HtmlPurifier/OpenFormsHtmlDefinition.php (1 hunks)
- api/config/purify.php (3 hunks)
- client/components/forms/MentionInput.vue (1 hunks)
- client/components/forms/RichTextAreaInput.client.vue (2 hunks)
- client/components/forms/TextBlock.vue (1 hunks)
- client/components/forms/components/FormSubmissionFormatter.js (1 hunks)
- client/components/forms/components/MentionDropdown.vue (1 hunks)
- client/components/open/forms/OpenCompleteForm.vue (3 hunks)
- client/components/open/forms/components/FirstSubmissionModal.vue (1 hunks)
- client/components/open/forms/components/form-components/FormSubmissionSettings.vue (1 hunks)
- client/components/open/integrations/SubmissionConfirmationIntegration.vue (1 hunks)
- client/data/blocks_types.json (1 hunks)
- client/lib/quill/quillMentionExtension.js (1 hunks)
🧰 Additional context used
🪛 Biome
client/lib/quill/quillMentionExtension.js
[error] 12-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 24-24: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (27)
api/app/Service/HtmlPurifier/OpenFormsHtmlDefinition.php (3)
1-8: LGTM: Namespace and imports are well-structured.The namespace follows the PSR-4 autoloading standard, and the imports are relevant to the class implementation.
9-11: LGTM: Class declaration and implementation are appropriate.The
OpenFormsHtmlDefinitionclass correctly implements theDefinitioninterface, ensuring compatibility with the Stevebauman Purify package. The use of a staticapplymethod is suitable for this utility-like class.
1-21: Overall assessment: Well-implemented custom HTML purification rules.The
OpenFormsHtmlDefinitionclass successfully extends the HTML5 definition with custom attributes for mentions and content editability. The implementation is correct, follows best practices, and integrates well with the Stevebauman Purify package. The code is clean and easy to understand, with only a minor suggestion for improved readability.client/components/open/forms/components/FirstSubmissionModal.vue (1)
5-5: Verify thebackdrop-blurprop usage and test visual appearance.The change from
backdrop-blur="sm"to:backdrop-blur="true"looks intentional, but there are a few points to consider:
Ensure that this change aligns with the
<modal>component's API. If the component has been updated to accept boolean values forbackdrop-blur, this change is correct.If the
<modal>component hasn't been updated, this change might cause unexpected behavior. In that case, you may need to revert to the string value or update the<modal>component to handle boolean values.Test the visual appearance of the modal with this change to confirm it produces the desired blur effect.
To verify the
<modal>component's implementation, you can run the following script:This script will help you verify if the
<modal>component supports boolean values for thebackdrop-blurprop.✅ Verification successful
Backdrop Blur Prop Correctly Updated to Boolean Value
The change from
backdrop-blur="sm"to:backdrop-blur="true"is appropriate and aligns with the<modal>component's API:
- The
<modal>component handlesbackdrop-bluras a boolean prop, controlling the application of thebackdrop-blur-smclass.- Updating the prop to a boolean ensures consistent and expected behavior of the backdrop blur effect.
- No further issues detected; the visual appearance should reflect the intended blur effect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the <modal> component implementation for backdrop-blur prop # Search for the modal component definition echo "Searching for modal component definition:" rg --type vue -g '!**/node_modules/**' -A 10 'export default .*modal' || echo "Modal component not found" # Search for backdrop-blur prop in the modal component echo "\nSearching for backdrop-blur prop in modal component:" rg --type vue -g '!**/node_modules/**' -A 5 'props.*backdrop-blur' || echo "backdrop-blur prop not found in modal component"Length of output: 574
Script:
#!/bin/bash # Description: Search for <modal> component and check backdrop-blur prop in .vue files # Find all .vue files excluding node_modules echo "Listing all .vue files:" fd --type f --extension vue --exclude node_modules # Search for <modal> component definition in .vue files echo "\nSearching for <modal> component definition:" fd --type f --extension vue --exclude node_modules | xargs rg '<modal' # Search for backdrop-blur prop within <modal> components echo "\nSearching for backdrop-blur prop within <modal> components:" fd --type f --extension vue --exclude node_modules | xargs rg 'backdrop-blur'Length of output: 14828
client/components/open/integrations/SubmissionConfirmationIntegration.vue (2)
27-27: Improved label clarity and consistency.The updates to the label text for various input fields enhance clarity and maintain consistency throughout the component. These changes improve the user experience without affecting functionality.
Also applies to: 36-36, 45-45, 52-52
30-37: Ensure proper input sanitization for email fields.While the changes enhance functionality, it's crucial to ensure that all user input, especially in the subject, reply-to, and email content fields, is properly sanitized before being used in emails. This helps prevent potential XSS attacks or email injection vulnerabilities.
To verify the security measures in place, please run the following script:
If no results are found, consider implementing appropriate input sanitization measures.
Also applies to: 41-42, 47-53
api/app/Integrations/Handlers/SubmissionConfirmationIntegration.php (2)
29-29: Verify the intention behind removing email validation for 'confirmation_reply_to'.The validation rule for 'confirmation_reply_to' has been changed from 'email' to 'nullable'. This allows the field to be either null or any value, potentially accepting invalid email addresses.
If the intention is to make the field optional while still ensuring valid email addresses when provided, consider using the following validation rule instead:
'confirmation_reply_to' => 'nullable|email'This change would allow the field to be null or a valid email address.
Could you please clarify the reasoning behind this change? If it's intentional, consider adding a comment explaining why email validation is no longer required for this field.
Line range hint
1-114: Consider updating tests and documentation.The changes made to this file include:
- Modifying the validation rule for 'confirmation_reply_to'
- Adding purification for 'notification_subject' in the formatData method
These changes, while minor, may require updates to existing unit tests to ensure they still pass with the new validation rules and data formatting. Additionally, any documentation referencing the 'confirmation_reply_to' field or the formatData method may need to be updated to reflect these changes.
Please ensure that:
- Existing unit tests are updated to accommodate the new validation rule for 'confirmation_reply_to'.
- Tests for the formatData method include checks for the purification of 'notification_subject'.
- Any relevant documentation is updated to reflect these changes.
api/config/purify.php (3)
3-3: LGTM: New use statement added correctly.The addition of the
usestatement forOpenFormsHtmlDefinitionis correct and consistent with its usage later in the file.
89-89: Verify implementation of OpenFormsHtmlDefinition class.The change from
Html5Definition::classtoOpenFormsHtmlDefinition::classis consistent with the new use statement. This custom class likely contains specific HTML purification rules for OpenForms.Please ensure that
OpenFormsHtmlDefinitionproperly extends or implements the necessary HTMLPurifier interfaces. Run the following script to verify the class implementation:✅ Verification successful
Verified: OpenFormsHtmlDefinition implements the Definition interface.
The
OpenFormsHtmlDefinitionclass correctly implements theDefinitioninterface, ensuring that the custom HTML purification rules for OpenForms are properly integrated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of OpenFormsHtmlDefinition class # Test: Check the OpenFormsHtmlDefinition class definition ast-grep --lang php --pattern $'class OpenFormsHtmlDefinition { $$$ }' # Test: Check if the class implements the required interface rg --type php -i 'class\s+OpenFormsHtmlDefinition.*implements.*Definition'Length of output: 422
Script:
#!/bin/bash # Description: Verify the implementation of OpenFormsHtmlDefinition class # Test: Check the OpenFormsHtmlDefinition class definition ast-grep --lang php --pattern $'class OpenFormsHtmlDefinition implements Definition {\n $$$\n}' # Test: Check if the class implements the Definition interface rg --type php -i "class\s+OpenFormsHtmlDefinition\s+implements\s+Definition"Length of output: 283
43-43: Verify security implications of new span attributes.The addition of new attributes for the
spantag, particularlymention-related attributes andcontenteditable, is noted. While this change supports the mention feature, it's crucial to ensure proper sanitization and validation of these attributes to prevent potential XSS vulnerabilities.Please confirm that proper sanitization and validation are in place for these new attributes, especially
contenteditable. Consider running the following script to check for any security measures:client/data/blocks_types.json (5)
8-9: LGTM: Consistent addition ofis_inputproperty for input types.The addition of
"is_input": trueto text, date, url, phone_number, and email input types is correct and consistent. This change will help in distinguishing input elements from non-input elements in the application.Also applies to: 17-18, 26-27, 35-36, 44-45
53-54: LGTM: Consistent addition ofis_inputproperty for complex input types.The addition of
"is_input": trueto checkbox, select, multi_select, and matrix input types is correct and consistent. This change maintains the pattern established for simpler input types and correctly categorizes these more complex input elements.Also applies to: 62-63, 71-72, 80-81
89-90: LGTM: Consistent addition ofis_inputproperty for numeric and special input types.The addition of
"is_input": trueto number, rating, scale, and slider input types is correct and consistent. This change appropriately categorizes these specialized input types, maintaining the overall schema consistency.Also applies to: 98-99, 107-108, 116-117
125-126: LGTM: Consistent addition ofis_inputproperty for file and signature input types.The addition of
"is_input": trueto files and signature input types is correct and consistent. This change properly categorizes these specialized input types, aligning them with the schema established for other input elements.Also applies to: 134-135
143-144: LGTM: Consistent addition ofis_inputproperty for non-input types.The addition of
"is_input": falseto nf-text, nf-page-break, nf-divider, nf-image, and nf-code types is correct and consistent. This change appropriately distinguishes these non-input elements from the input types.Overall, the changes in this file provide a clear and consistent way to differentiate between input and non-input elements. This addition will likely improve the application's ability to handle different types of form elements, potentially simplifying form processing, validation, or UI rendering logic.
Also applies to: 152-153, 161-162, 170-171, 179-180
client/components/forms/TextBlock.vue (1)
8-8: Verify the import path forFormSubmissionFormatter.Ensure that the import statement for
FormSubmissionFormatteruses the correct relative path. Given the current file location, the path'./components/FormSubmissionFormatter'may not resolve correctly if thecomponentsdirectory is not a sibling of the current file's directory.You can verify the correct path with the following command:
✅ Verification successful
Import path for
FormSubmissionFormatteris correct.The import statement
./components/FormSubmissionFormatterinTextBlock.vuecorrectly resolves toclient/components/forms/components/FormSubmissionFormatter.js.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct import path for FormSubmissionFormatter. # Search for files named FormSubmissionFormatter.js or FormSubmissionFormatter.vue in the project. fd --type f --name 'FormSubmissionFormatter.*'Length of output: 299
Script:
#!/bin/bash # Description: Correctly verify the presence of FormSubmissionFormatter.js or FormSubmissionFormatter.vue. # Search for files named FormSubmissionFormatter.js or FormSubmissionFormatter.vue in the project. fd --type f 'FormSubmissionFormatter.{js,vue}'Length of output: 392
Script:
#!/bin/bash # Description: Correctly verify the presence of FormSubmissionFormatter.js or FormSubmissionFormatter.vue using glob pattern. # Search for files named FormSubmissionFormatter.js or FormSubmissionFormatter.vue in the project using a glob pattern. fd --type f --glob 'FormSubmissionFormatter.{js,vue}'Length of output: 117
api/app/Open/MentionParser.php (1)
49-51: Verify the handling of object values in thegetDatamethodIn the
getDatamethod, when$valueis an object, it is cast to an array:if (is_object($value)) { return (array) $value; }Ensure that casting the object to an array aligns with the intended usage and that the rest of your codebase can handle
$valueas an array. If this is not the desired behavior, consider adjusting how object values are processed.client/lib/quill/quillMentionExtension.js (2)
13-71: Usage of static methods inMentionBlotis appropriateThe
MentionBlotclass consists of static properties and methods, which aligns with Quill's requirements for custom blot implementations. This structure allows Quill to properly manage and serialize the blot elements.🧰 Tools
🪛 Biome
[error] 12-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 24-24: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
91-91: Ensure the 'mention' module is correctly registeredThe condition checks if the 'mention' module is not already registered. Make sure that this check accurately reflects the module's registration status to prevent multiple registrations.
Run the following script to confirm that the 'mention' module is registered only once:
✅ Verification successful
'mention' module is correctly registered only once
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the 'mention' module is registered correctly. # Test: Search for all registrations of 'modules/mention'. Expect: Only one registration. rg --type js "Quill\.register\('modules/mention'" -A 2Length of output: 246
client/components/forms/components/MentionDropdown.vue (1)
2-68: EnsureUPopoverandUButtoncomponents are imported or registeredThe components
UPopoverandUButtonare used in the template but not imported in the script. If these components are not globally registered, you need to import them to prevent runtime errors.If
UPopoverandUButtonare not globally available, import them:+import UPopover from '@/components/UPopover.vue' +import UButton from '@/components/UButton.vue'client/components/forms/RichTextAreaInput.client.vue (4)
40-45: LGTMThe
MentionDropdowncomponent is correctly integrated with the appropriate props and conditional rendering.
53-54: LGTMThe necessary imports for
MentionDropdownandregisterMentionExtensionare correctly added.
60-60: LGTMThe
MentionDropdowncomponent is properly included in the components object.
166-168: LGTMThe styling for mentions is implemented appropriately using Tailwind CSS utility classes.
client/components/open/forms/components/form-components/FormSubmissionSettings.vue (1)
132-133: Mention functionality added successfullyThe addition of the
enable-mentionsprop and bindingform.propertiesto the:mentionsprop in therich-text-area-inputcomponent correctly integrates the mention functionality into the success page text.client/components/open/forms/OpenCompleteForm.vue (1)
269-269: Confirm that storingform.data()does not expose sensitive informationAssigning
form.data()tosubmittedDatamay include sensitive user information in the component's state. Ensure thatform.data()does not contain any personally identifiable information (PII) or sensitive data that should not be stored or displayed.If necessary, consider storing only the required non-sensitive fields:
- this.submittedData = form.data() + // Extract only necessary non-sensitive fields + const { field1, field2 } = form.data() + this.submittedData = { field1, field2 }
| @@ -0,0 +1,49 @@ | |||
| <template> | |||
| <div | |||
| v-html="processedContent" | |||
There was a problem hiding this comment.
Sanitize processedContent before rendering with v-html to prevent XSS vulnerabilities.
Rendering raw HTML content using v-html can introduce security risks if the content is not properly sanitized. To prevent potential cross-site scripting (XSS) attacks, ensure that processedContent is sanitized before rendering.
Consider using a library like DOMPurify to sanitize the HTML content.
| return props.content.replace(/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/g, (match, fieldId, fallback) => { | ||
| const value = formattedData[fieldId] | ||
| if (value) { | ||
| if(Array.isArray(value)) { | ||
| return value.map(v => `<span>${v}</span>`).join(' ') | ||
| } | ||
| return `<span>${value}</span>` | ||
| } else if (fallback) { | ||
| return `<span>${fallback}</span>` | ||
| } | ||
| return '' | ||
| }) |
There was a problem hiding this comment.
Escape dynamic values to prevent XSS vulnerabilities when injecting into HTML.
The dynamic values value and fallback are inserted directly into HTML without escaping. If these values contain user-generated content, this can lead to XSS vulnerabilities. It's important to escape these values before inserting them into the HTML.
Apply this diff to ensure values are properly escaped:
+import { escape } from 'he';
...
return props.content.replace(/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/g, (match, fieldId, fallback) => {
const value = formattedData[fieldId]
if (value) {
if(Array.isArray(value)) {
- return value.map(v => `<span>${v}</span>`).join(' ')
+ return value.map(v => `<span>${escape(String(v))}</span>`).join(' ')
}
- return `<span>${value}</span>`
+ return `<span>${escape(String(value))}</span>`
} else if (fallback) {
- return `<span>${fallback}</span>`
+ return `<span>${escape(String(fallback))}</span>`
}
return ''
})This uses the escape function from the he library to safely encode any HTML entities.
📝 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.
| return props.content.replace(/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/g, (match, fieldId, fallback) => { | |
| const value = formattedData[fieldId] | |
| if (value) { | |
| if(Array.isArray(value)) { | |
| return value.map(v => `<span>${v}</span>`).join(' ') | |
| } | |
| return `<span>${value}</span>` | |
| } else if (fallback) { | |
| return `<span>${fallback}</span>` | |
| } | |
| return '' | |
| }) | |
| import { escape } from 'he'; | |
| return props.content.replace(/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/g, (match, fieldId, fallback) => { | |
| const value = formattedData[fieldId] | |
| if (value) { | |
| if(Array.isArray(value)) { | |
| return value.map(v => `<span>${escape(String(v))}</span>`).join(' ') | |
| } | |
| return `<span>${escape(String(value))}</span>` | |
| } else if (fallback) { | |
| return `<span>${escape(String(fallback))}</span>` | |
| } | |
| return '' | |
| }) |
api/app/Open/MentionParser.php
Outdated
| $value = collect($this->data)->filter(function ($item) use ($fieldId) { | ||
| return $item['id'] === $fieldId; | ||
| })->first()['value'] ?? null; |
There was a problem hiding this comment.
Ensure the collect helper function is accessible in this context
The code uses the collect helper function, which may not be available without the proper import or dependency. To ensure the function operates correctly, consider importing the necessary class or function. If you're using Laravel, add the following import at the top of your file:
use Illuminate\Support\Arr;Alternatively, if you prefer to use the global helper function, ensure that it is accessible in your application context.
| public function __construct($content, $data) | ||
| { | ||
| $this->content = $content; | ||
| $this->data = $data; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and return types for improved code clarity
Adding parameter and return type hints enhances readability and helps catch type-related bugs. Consider updating your methods with type declarations:
-public function __construct($content, $data)
+public function __construct(string $content, array $data)
-public function parse()
+public function parse(): string
-private function replaceMentions()
+private function replaceMentions(): string
-private function getData($fieldId)
+private function getData(string $fieldId)Also applies to: 16-20, 21-41, 43-54
api/app/Open/MentionParser.php
Outdated
| { | ||
| $value = collect($this->data)->filter(function ($item) use ($fieldId) { | ||
| return $item['id'] === $fieldId; | ||
| })->first()['value'] ?? null; |
There was a problem hiding this comment.
Prevent potential null dereference when accessing ['value']
The expression ->first()['value'] ?? null may cause an error if first() returns null, because you cannot access ['value'] on null. Modify the code to safely handle this case:
-$value = collect($this->data)->filter(function ($item) use ($fieldId) {
- return $item['id'] === $fieldId;
-})->first()['value'] ?? null;
+$firstItem = collect($this->data)->filter(function ($item) use ($fieldId) {
+ return $item['id'] === $fieldId;
+})->first();
+$value = $firstItem['value'] ?? null;📝 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.
| })->first()['value'] ?? null; | |
| $firstItem = collect($this->data)->filter(function ($item) use ($fieldId) { | |
| return $item['id'] === $fieldId; | |
| })->first(); | |
| $value = $firstItem['value'] ?? null; |
| setup(props, context) { | ||
| const editorOptions = { | ||
| ...props.editorOptions, | ||
| modules: { | ||
| ...props.editorOptions.modules, | ||
| mention: props.enableMentions ? { mentions: props.mentions } : undefined | ||
| } | ||
| } | ||
| const editorToolbar = props.enableMentions | ||
| ? [...props.editorToolbar, ['mention']] | ||
| : props.editorToolbar | ||
| return { | ||
| ...useFormInput(props, context) | ||
| ...useFormInput(props, context), | ||
| editorOptions, | ||
| editorToolbar, | ||
| mentionState: registerMentionExtension(Quill) |
There was a problem hiding this comment.
Include 'mention' in editorOptions when mentions are enabled
After removing 'mention' from the default editorOptions, ensure that 'mention' is included in editorOptions.formats and the mention module is added when enableMentions is true. This ensures the editor supports the mention functionality correctly.
Apply this change to the setup function:
setup(props, context) {
+ const formats = props.enableMentions
+ ? [...props.editorOptions.formats, 'mention']
+ : props.editorOptions.formats;
const editorOptions = {
...props.editorOptions,
+ formats: formats,
modules: {
...props.editorOptions.modules,
mention: props.enableMentions ? { mentions: props.mentions } : undefined
}
}
const editorToolbar = props.enableMentions
? [...props.editorToolbar, ['mention']]
: props.editorToolbar
return {
...useFormInput(props, context),
editorOptions,
editorToolbar,
mentionState: registerMentionExtension(Quill)
}
}📝 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.
| setup(props, context) { | |
| const editorOptions = { | |
| ...props.editorOptions, | |
| modules: { | |
| ...props.editorOptions.modules, | |
| mention: props.enableMentions ? { mentions: props.mentions } : undefined | |
| } | |
| } | |
| const editorToolbar = props.enableMentions | |
| ? [...props.editorToolbar, ['mention']] | |
| : props.editorToolbar | |
| return { | |
| ...useFormInput(props, context) | |
| ...useFormInput(props, context), | |
| editorOptions, | |
| editorToolbar, | |
| mentionState: registerMentionExtension(Quill) | |
| setup(props, context) { | |
| const formats = props.enableMentions | |
| ? [...props.editorOptions.formats, 'mention'] | |
| : props.editorOptions.formats; | |
| const editorOptions = { | |
| ...props.editorOptions, | |
| formats: formats, | |
| modules: { | |
| ...props.editorOptions.modules, | |
| mention: props.enableMentions ? { mentions: props.mentions } : undefined | |
| } | |
| } | |
| const editorToolbar = props.enableMentions | |
| ? [...props.editorToolbar, ['mention']] | |
| : props.editorToolbar | |
| return { | |
| ...useFormInput(props, context), | |
| editorOptions, | |
| editorToolbar, | |
| mentionState: registerMentionExtension(Quill) | |
| } | |
| } |
| const updateCompVal = () => { | ||
| compVal.value = editableDiv.value.innerHTML | ||
| } |
There was a problem hiding this comment.
Sanitize user input to prevent XSS vulnerabilities
In updateCompVal, you're assigning editableDiv.value.innerHTML directly to compVal.value, and vice versa. This can introduce XSS vulnerabilities if malicious content is inserted into the editableDiv. To mitigate this, consider sanitizing the HTML content before using it. Utilizing a library like DOMPurify can help sanitize user input.
Apply this diff to implement sanitization:
+import DOMPurify from 'dompurify'
...
const updateCompVal = () => {
- compVal.value = editableDiv.value.innerHTML
+ compVal.value = DOMPurify.sanitize(editableDiv.value.innerHTML)
}
...
onMounted(() => {
if (compVal.value) {
- editableDiv.value.innerHTML = compVal.value
+ editableDiv.value.innerHTML = DOMPurify.sanitize(compVal.value)
}
})
...
watch(compVal, (newVal) => {
if (editableDiv.value && editableDiv.value.innerHTML !== newVal) {
- editableDiv.value.innerHTML = newVal
+ editableDiv.value.innerHTML = DOMPurify.sanitize(newVal)
}
})Also applies to: 144-147, 148-152
| const range = window.getSelection().getRangeAt(0) | ||
| // Insert the mention span | ||
| range.insertNode(mentionSpan) |
There was a problem hiding this comment.
Add null checks for selection and range objects
In functions like insertMention and restoreSelection, window.getSelection() and getRangeAt(0) may return null or cause errors if no selection exists. To prevent runtime exceptions, add null checks to ensure these objects are valid before using them.
Apply this diff to add null checks:
const insertMention = (mention) => {
const mentionSpan = createMentionSpan(mention)
restoreSelection()
+ const selection = window.getSelection()
+ if (!selection || selection.rangeCount === 0) {
+ return
+ }
- const range = window.getSelection().getRangeAt(0)
+ const range = selection.getRangeAt(0)
// Insert the mention span
range.insertNode(mentionSpan)
// ...
}
const restoreSelection = () => {
if (savedRange.value) {
+ const selection = window.getSelection()
+ if (!selection) {
+ return
+ }
selection.removeAllRanges()
selection.addRange(savedRange.value)
editableDiv.value.focus()
}
}Also applies to: 131-135
| min-height: 1.5rem; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| } | ||
| .mention-input:empty::before { | ||
| content: attr(placeholder); | ||
| color: #9ca3af; | ||
| } |
There was a problem hiding this comment.
Ensure placeholder text is accessible
Using the ::before pseudo-element to display placeholder text may not be accessible to screen readers, impacting users relying on assistive technologies. Consider using the placeholder attribute or adding appropriate ARIA attributes to improve accessibility.
Apply this diff to enhance accessibility:
-.mention-input:empty::before {
- content: attr(placeholder);
- color: #9ca3af;
-}
+/* Remove the pseudo-element and use a data attribute */
+<div
+ ref="editableDiv"
+ :contenteditable="!disabled"
+ class="mention-input"
+ :style="inputStyle"
+ :class="[ /* existing classes */ ]"
+ @input="onInput"
+ :data-placeholder="placeholder"
+ role="textbox"
+ aria-multiline="true"
+/>Update the CSS:
.mention-input:empty::before {
content: attr(data-placeholder);
color: #9ca3af;
}| <TextBlock | ||
| v-if="form.submitted_text" | ||
| class="form-description text-gray-700 dark:text-gray-300 whitespace-pre-wrap" | ||
| v-html="form.submitted_text " | ||
| :content="form.submitted_text" | ||
| :mentions-allowed="true" | ||
| :form="form" | ||
| :form-data="submittedData" |
There was a problem hiding this comment.
Ensure submittedData is reset when restarting the form to prevent data leakage
Currently, the submittedData property is not reset when the form is restarted. This could lead to previous submission data being unintentionally displayed if the user restarts and resubmits the form. To prevent potential data leakage, consider resetting submittedData to null in the restart method.
Apply this diff to reset submittedData:
def restart () {
this.submitted = false
this.$emit('restarted', true)
+ this.submittedData = null
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/app/Mail/Forms/SubmissionConfirmationMail.php (4 hunks)
🧰 Additional context used
🔇 Additional comments (7)
api/app/Mail/Forms/SubmissionConfirmationMail.php (7)
7-7: ImportingMentionParserThe
MentionParseris appropriately imported and utilized in the class methods for parsing dynamic content.
20-20: Adding$formattedDataPropertyThe private property
$formattedDatais correctly added to store the formatted submission data.
29-34: Initializing$formattedDatawithFormSubmissionFormatterThe
FormSubmissionFormatteris effectively used to initialize$formattedData, ensuring that the submission data is properly formatted for the email.
50-50: UtilizinggetSubject()inbuild()MethodThe
getSubject()method is correctly called to set the email subject dynamically.
52-52: Passing Formatted Data to the ViewThe
$formattedDatais appropriately passed to the email view, ensuring that the email content has access to the formatted fields.
75-80: Parsing and Validating Reply-To EmailThe
getReplyToEmail()method correctly parses the reply-to email usingMentionParserand validates it before use.
85-89: ImplementinggetSubject()MethodThe
getSubject()method is well-implemented, utilizingMentionParserto dynamically generate the email subject based on the integration data.
| private function getNotificationBody() | ||
| { | ||
| $parser = new MentionParser($this->integrationData->notification_body, $this->formattedData); | ||
| return $parser->parse(); | ||
| } |
There was a problem hiding this comment.
Review the Usage of getNotificationBody()
The getNotificationBody() method is defined but not used within this class. Unused methods can introduce unnecessary complexity. Consider implementing it where appropriate or removing it if it's not needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
api/app/Mail/Forms/SubmissionConfirmationMail.php (2)
92-96: Good addition, but consider refactoringThe
getNotificationBodymethod is a good addition and is now properly used in thebuildmethod, addressing a previous review comment. However, like thegetSubjectmethod, it contains repetitive parsing logic.Consider applying the same refactoring suggestion as for
getSubject:private function getNotificationBody() { return $this->parseIntegrationDataField('notification_body') ?? ''; }This refactoring would reduce code duplication and improve maintainability.
Line range hint
1-96: Overall assessment: Good improvements with room for refinementThe changes to
SubmissionConfirmationMailclass significantly enhance its functionality by introducing mention parsing and pre-formatting of submission data. Key improvements include:
- Addition of
MentionParserfor dynamic content in subjects, bodies, and reply-to addresses.- Pre-formatting of submission data in the constructor for efficiency.
- Proper use of parsed notification body in the email view.
To further improve the code:
- Implement exception handling in the constructor when initializing
FormSubmissionFormatter.- Refactor the repetitive parsing logic in
getSubject,getNotificationBody, andgetReplyToEmailinto a single method.These refinements will enhance the robustness and maintainability of the class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/app/Mail/Forms/SubmissionConfirmationMail.php (4 hunks)
🧰 Additional context used
🔇 Additional comments (6)
api/app/Mail/Forms/SubmissionConfirmationMail.php (6)
7-7: LGTM: New import for MentionParserThe addition of the
MentionParserimport is appropriate for the new functionality implemented in this class.
20-21: LGTM: New private property for formatted dataThe addition of the
$formattedDataproperty is a good practice. It centralizes the storage of formatted submission data, which is then used across multiple methods in the class.
50-53: LGTM: Improved email building with parsed contentThe changes in the
buildmethod are good improvements:
- Using
getSubject()for the email subject allows for dynamic, parsed content.- Adding
notificationBodyto the view data addresses a previous review comment about unused parsed content.- Using
$this->formattedDatafor fields is consistent with the new pre-formatting approach.These changes enhance the email's content with parsed and formatted data.
76-81: LGTM: Improved reply-to email handlingThe changes in
getReplyToEmailmethod are well-implemented:
- It now uses
MentionParserto process the reply-to email, allowing for dynamic content.- The parsed email is properly validated using
filter_var, ensuring only valid email addresses are used.- The method falls back to the default email if parsing or validation fails, maintaining robustness.
This implementation enhances the flexibility of the reply-to feature while maintaining email validity.
29-34:⚠️ Potential issueConsider adding exception handling
The initialization of
FormSubmissionFormatteris good for efficiency, but it lacks exception handling. As mentioned in a previous review, consider wrapping this in a try-catch block to handle potential exceptions gracefully.Example:
try { $formatter = (new FormSubmissionFormatter($event->form, $event->data)) ->createLinks() ->outputStringsOnly() ->useSignedUrlForFiles(); $this->formattedData = $formatter->getFieldsWithValue(); } catch (\Exception $e) { // Handle the exception, log it, or set a default value $this->formattedData = []; \Log::error('FormSubmissionFormatter failed: ' . $e->getMessage()); }
86-90: 🛠️ Refactor suggestionConsider refactoring repetitive parsing logic
While the
getSubjectmethod is well-implemented and allows for dynamic subjects, there's repetitive parsing logic across this and other methods (likegetNotificationBody). As suggested in a previous review, consider refactoring this common logic into a single private method to adhere to the DRY principle.Example refactored method:
private function parseIntegrationDataField($field) { $value = $this->integrationData->$field ?? null; if ($value) { $parser = new MentionParser($value, $this->formattedData); return $parser->parse(); } return null; }Then you could simplify
getSubjectto:private function getSubject() { return $this->parseIntegrationDataField('notification_subject') ?? ''; }
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
client/package.json (1)
Line range hint
1-89: Consider the implications of switching rich text editorsThe switch from vue3-editor to Quill represents a significant change in the project's rich text editing capabilities. While this change is likely to bring improvements, it's crucial to ensure that:
- All components previously using vue3-editor are updated to use Quill.
- Any custom functionality built around vue3-editor is recreated or adapted for Quill.
- The application's documentation is updated to reflect this change.
- Developers are informed about this change and provided resources to work with Quill if needed.
Consider creating a wrapper component for Quill to centralize its configuration and make future editor changes easier to manage.
client/lib/quill/quillMentionExtension.js (1)
69-71: Add a comment explaining thelengthmethodThe
lengthmethod always returns 1. While this is likely intentional, it's not immediately clear why. Consider adding a comment to explain the reasoning.Here's a suggested change:
length() { + // A mention is treated as a single unit, regardless of its content length return 1 }This comment helps future maintainers understand the design decision behind this method.
api/tests/Unit/Service/Forms/MentionParserTest.php (4)
109-127: LGTM: Good test for handling array values, with a minor suggestion.This test case effectively verifies that the MentionParser can handle array values, joining them into a comma-separated string. It's a valuable test for ensuring flexibility in data types.
Consider adding an additional assertion to check that the array values are trimmed and that there's no trailing comma. This would ensure more precise handling of array values.
155-173: LGTM: Good test for UTF-8 character handling, with a suggestion for improvement.This test case effectively verifies that the MentionParser can handle UTF-8 characters correctly. This is important for ensuring proper internationalization support.
Consider adding more diverse UTF-8 characters in both the content and the replacement value to ensure a wider range of character encodings are properly handled.
178-196: LGTM: Good test for handling content without paragraph tags, with a suggestion.This test case effectively verifies that the MentionParser can handle content without surrounding paragraph tags, which is important for flexibility in input formats.
Consider adding an additional test case where the content has mixed inline and block-level elements to ensure the parser handles more complex HTML structures correctly.
1-196: Excellent test suite with comprehensive coverage.The MentionParserTest provides a robust set of test cases that cover various aspects of the MentionParser functionality. The tests are well-structured, clear, and effectively verify the parser's behavior in different scenarios.
To further enhance the test suite, consider adding the following test cases:
- Test with nested mentions (mentions within mentions).
- Test with very large input content to ensure performance.
- Test with malformed HTML to ensure the parser handles errors gracefully.
- Test with mentions that have special characters in their field IDs.
- Test the parser's behavior when the same mention appears multiple times in the content.
These additional tests would help ensure even more robust and reliable functionality of the MentionParser class.
client/components/forms/RichTextAreaInput.client.vue (1)
82-106: Flexible Quill options configurationThe
quillOptionscomputed property provides a flexible way to configure the Quill editor. The merging of default options with user-provided options and the conditional addition of mention-related configurations is well-implemented.However, consider extracting the default options to a separate constant or configuration file to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- api/app/Open/MentionParser.php (1 hunks)
- api/config/purify.php (3 hunks)
- api/tests/Unit/Service/Forms/MentionParserTest.php (1 hunks)
- client/components/forms/RichTextAreaInput.client.vue (3 hunks)
- client/components/forms/components/QuillyEditor.vue (1 hunks)
- client/composables/lib/vForm/Form.js (1 hunks)
- client/lib/quill/quillMentionExtension.js (1 hunks)
- client/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/config/purify.php
🧰 Additional context used
🪛 Biome
client/lib/quill/quillMentionExtension.js
[error] 11-11: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (20)
api/app/Open/MentionParser.php (3)
8-17: LGTM! Consider implementing type hints as suggested previously.The class structure looks good. However, as mentioned in a previous review, consider adding type hints to the constructor parameters for improved code clarity and type safety.
1-97: Overall solid implementation with room for enhancement.The
MentionParserclass provides a robust solution for parsing and replacing mentions in content. Key strengths include:
- Careful handling of XML/HTML parsing and encoding.
- Flexible mention replacement logic.
Areas for improvement:
- Add type hints to method parameters and return types.
- Refactor the
parse()method into smaller, more focused methods.- Add error handling for DOM operations.
- Clarify the purpose of or remove the unused
replaceMentions()method.- Address the potential null dereference in
getData().These enhancements will improve code clarity, maintainability, and robustness.
87-96:⚠️ Potential issueAddress potential issues in the
getData()method.
Ensure the
collect()helper function is properly imported or available in this context. Consider addinguse Illuminate\Support\Collection;at the top of the file if you're using Laravel's Collection.As mentioned in a previous review, there's a potential null dereference issue when accessing
['value']. Consider refactoring to safely handle this case:private function getData($fieldId) { $item = collect($this->data)->firstWhere('nf_id', $fieldId); $value = $item['value'] ?? null; if (is_object($value)) { return (array) $value; } return $value; }This change ensures that
['value']is only accessed if$itemis not null.client/lib/quill/quillMentionExtension.js (2)
5-130: Well-structured mention extension implementationThe
registerMentionExtensionfunction is well-implemented. It encapsulates the mention functionality, registers the necessary components with Quill, and provides a reactive state for managing the mention interface. This approach allows for easy integration and management of the mention feature in the Quill editor.Great job on structuring this extension! It's modular, follows Quill's extension patterns, and provides a clean interface for use in the application.
🧰 Tools
🪛 Biome
[error] 11-11: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
1-130: Overall assessment: Well-implemented Quill mention extensionThis file introduces a well-structured and comprehensive mention extension for the Quill editor. The implementation follows Quill's extension patterns and provides a clean interface for integration. A few minor improvements have been suggested:
- Refactoring the
static createmethod inMentionBlotfor clarity.- Adding a comment to explain the
lengthmethod inMentionBlot.- Ensuring proper import or definition of
nextTickin theMentionModule.These small changes will enhance the clarity and maintainability of the code. Great work on implementing this complex functionality!
🧰 Tools
🪛 Biome
[error] 11-11: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
api/tests/Unit/Service/Forms/MentionParserTest.php (5)
11-29: LGTM: Well-structured test for basic mention replacement.This test case effectively covers the basic functionality of replacing a single mention element with its corresponding value. The setup, execution, and assertion are clear and concise.
34-58: LGTM: Comprehensive test for handling multiple mentions.This test case effectively verifies the MentionParser's ability to handle multiple mentions within the same content string. It ensures that each mention is correctly replaced with its corresponding value.
63-81: LGTM: Proper testing of fallback functionality.This test case effectively verifies that the MentionParser uses the fallback value when the mentioned value is not found in the provided data. It's a crucial test for ensuring graceful handling of missing data.
86-104: LGTM: Proper handling of edge case with no value and no fallback.This test case effectively verifies that the MentionParser removes the mention element entirely when there's no corresponding value in the data and no fallback provided. This is an important edge case to cover.
132-150: LGTM: Excellent test for HTML structure preservation.This test case effectively verifies that the MentionParser preserves the surrounding HTML structure while replacing mentions. This is crucial for maintaining the integrity of the original content layout.
client/components/forms/RichTextAreaInput.client.vue (6)
7-32: Improved editor structure and stylingThe new wrapper
divwith conditional classes and theQuillyEditorcomponent provide better control over the editor's appearance and behavior. The use of dynamic classes based on component state (e.g., error, disabled) is a good practice for maintaining consistent UI feedback.
41-45: Conditional rendering of MentionDropdownThe conditional rendering of the
MentionDropdowncomponent based onenableMentionsandmentionStateis a good approach. It ensures that the mention functionality is only available when explicitly enabled and properly initialized.
50-56: Updated imports and component structureThe new imports and component structure align well with the template changes. The introduction of
QuillyEditorandMentionDropdowncomponents, along with the Quill library and mention extension, provides a solid foundation for the rich text editor functionality.
57-71: Props update for mention functionalityThe addition of
enableMentionsandmentionsprops provides a clean way to control the mention functionality. The default values are appropriate, allowing for easy opt-in to the feature.
76-81: Mention extension registration in onMountedMoving the mention extension registration to the
onMountedhook is a good practice. It ensures that the extension is only registered when necessary and avoids potential issues with SSR or component initialization.
115-115: Improved styling and responsivenessThe use of a CSS variable for font size (
--font-size) and the addition of styles for the Quill header and placeholder improve the overall look and feel of the editor. This approach allows for easier theming and responsiveness.Also applies to: 125-130
client/composables/lib/vForm/Form.js (1)
92-92: Improved deep cloning implementationThe change from
JSON.parse(JSON.stringify(this.originalData[key]))tocloneDeep(this.originalData[key])is a good improvement. Here's why:
- It eliminates the limitation of
JSON.parse(JSON.stringify()), which can't handle circular references or certain JavaScript types (likeDate,RegExp,Map,Set, etc.).cloneDeepfrom the 'clone-deep' library is designed to handle complex objects more robustly, including nested structures and special JavaScript types.- This change makes the code more reliable and less prone to unexpected behavior when dealing with complex data structures.
Consider adding a comment explaining why
cloneDeepis used here, to help future maintainers understand the importance of deep cloning in this context. For example:// Use cloneDeep to ensure a true deep copy, handling complex nested structures this[key] = cloneDeep(this.originalData[key])To ensure that
cloneDeepis being used consistently throughout the codebase for similar scenarios, you can run the following script:This script will help identify any remaining instances of
JSON.parse(JSON.stringify())that might benefit from being replaced withcloneDeep, and also check for consistent import and usage ofcloneDeepacross the JavaScript files in the project.client/components/forms/components/QuillyEditor.vue (3)
1-6: Template structure is properly set upThe template correctly defines the container for the Quill editor with the necessary
refand class attributes.
13-22: Props are correctly defined with appropriate defaultsThe
modelValueandoptionsprops are well-defined, ensuring that the component receives the necessary data and configurations.
24-32: Emits are properly declared for event handlingThe component accurately declares all the events it emits, facilitating effective communication with parent components.
| public function parse() | ||
| { | ||
| $doc = new DOMDocument(); | ||
| // Disable libxml errors and use internal errors | ||
| $internalErrors = libxml_use_internal_errors(true); | ||
|
|
||
| // Wrap the content in a root element to ensure it's valid XML | ||
| $wrappedContent = '<root>' . $this->content . '</root>'; | ||
|
|
||
| // Load HTML, using UTF-8 encoding | ||
| $doc->loadHTML(mb_convert_encoding($wrappedContent, 'HTML-ENTITIES', 'UTF-8'), LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD); | ||
|
|
||
| // Restore libxml error handling | ||
| libxml_use_internal_errors($internalErrors); | ||
|
|
||
| $xpath = new DOMXPath($doc); | ||
| $mentionElements = $xpath->query("//span[@mention]"); | ||
|
|
||
| foreach ($mentionElements as $element) { | ||
| $fieldId = $element->getAttribute('mention-field-id'); | ||
| $fallback = $element->getAttribute('mention-fallback'); | ||
| $value = $this->getData($fieldId); | ||
|
|
||
| if ($value !== null) { | ||
| $textNode = $doc->createTextNode(is_array($value) ? implode(', ', $value) : $value); | ||
| $element->parentNode->replaceChild($textNode, $element); | ||
| } elseif ($fallback) { | ||
| $textNode = $doc->createTextNode($fallback); | ||
| $element->parentNode->replaceChild($textNode, $element); | ||
| } else { | ||
| $element->parentNode->removeChild($element); | ||
| } | ||
| } | ||
|
|
||
| // Extract and return the processed HTML content | ||
| $result = $doc->saveHTML($doc->getElementsByTagName('root')->item(0)); | ||
|
|
||
| // Remove the root tags we added | ||
| $result = preg_replace('/<\/?root>/', '', $result); | ||
|
|
||
| // Trim whitespace and convert HTML entities back to UTF-8 characters | ||
| $result = trim(html_entity_decode($result, ENT_QUOTES | ENT_HTML5, 'UTF-8')); | ||
|
|
||
| return $result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring and adding error handling.
The parse() method is well-structured but quite long. Consider breaking it down into smaller, more focused methods for better readability and maintainability. Additionally, it would be beneficial to add error handling for potential DOM manipulation failures.
For example:
- Create a method for initializing the DOMDocument.
- Create a method for processing mention elements.
- Create a method for extracting the final HTML content.
Also, consider wrapping the DOM operations in a try-catch block to handle any potential exceptions.
| private function replaceMentions() | ||
| { | ||
| $pattern = '/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/'; | ||
| return preg_replace_callback($pattern, function ($matches) { | ||
| $fieldId = $matches[1]; | ||
| $fallback = $matches[2]; | ||
| $value = $this->getData($fieldId); | ||
|
|
||
| if ($value !== null) { | ||
| if (is_array($value)) { | ||
| return implode(' ', array_map(function ($v) { | ||
| return $v; | ||
| }, $value)); | ||
| } | ||
| return $value; | ||
| } elseif ($fallback) { | ||
| return $fallback; | ||
| } | ||
| return ''; | ||
| }, $this->content); | ||
| } |
There was a problem hiding this comment.
Clarify the purpose of the unused replaceMentions() method.
The replaceMentions() method appears to be an alternative implementation using regex for parsing mentions. However, it's not currently used in the class. Consider either:
- Removing this method if it's no longer needed.
- Documenting its purpose if it's intended for future use or as an alternative implementation.
- Refactoring the class to use this method if it's meant to be the primary implementation.
Keeping unused methods can lead to confusion and maintenance issues in the future.
| nextTick(() => { | ||
| // Focus the editor | ||
| this.quill.focus() | ||
|
|
||
| // Set the selection after the mention | ||
| this.quill.setSelection(index + mentionLength, 0, Quill.sources.SILENT) | ||
| }) |
There was a problem hiding this comment.
Import or define nextTick
The insertMention method uses nextTick, but this function is not imported or defined in the current file. Ensure that nextTick is properly imported or defined.
If you're using Vue 3, you can import nextTick from 'vue'. Add the following import at the top of the file:
import { reactive, nextTick } from 'vue'If nextTick is defined elsewhere in your project, make sure it's properly imported or accessible in this file.
| static create(data) { | ||
| let node = super.create() | ||
| MentionBlot.setAttributes(node, data) | ||
| return node | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the static create method
The use of super.create() in a static context can be confusing. Consider refactoring this to use the class name explicitly.
Here's a suggested refactoring:
static create(data) {
- let node = super.create()
+ let node = Inline.create()
MentionBlot.setAttributes(node, data)
return node
}This change makes it clear that we're calling the create method of the Inline class.
📝 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.
| static create(data) { | |
| let node = super.create() | |
| MentionBlot.setAttributes(node, data) | |
| return node | |
| } | |
| static create(data) { | |
| let node = Inline.create() | |
| MentionBlot.setAttributes(node, data) | |
| return node | |
| } |
🧰 Tools
🪛 Biome
[error] 11-11: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
| .ql-mention { | ||
| padding-top: 0px !important; | ||
| } | ||
| .ql-mention::after { | ||
| content: '@'; | ||
| font-size: 16px; | ||
| } | ||
| .rich-editor { | ||
| span[mention] { | ||
| @apply inline-flex items-center align-baseline leading-tight text-sm relative bg-blue-100 text-blue-800 border border-blue-200 rounded-md px-1 py-0.5 mx-0.5; | ||
| max-width: 200px; | ||
| white-space: nowrap; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| cursor: default; | ||
| } | ||
| } |
There was a problem hiding this comment.
Styles for mentions
The added styles for mentions provide a clear visual distinction for mentioned items. The use of Tailwind classes for styling is consistent with the project's approach.
However, the ::after pseudo-element for adding the '@' symbol before mentions should be changed to ::before for better semantic meaning and consistency with common UI patterns.
Apply this diff to fix the issue:
-.ql-mention::after {
+.ql-mention::before {
content: '@';
- font-size: 16px;
+ font-size: 14px;
}📝 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.
| .ql-mention { | |
| padding-top: 0px !important; | |
| } | |
| .ql-mention::after { | |
| content: '@'; | |
| font-size: 16px; | |
| } | |
| .rich-editor { | |
| span[mention] { | |
| @apply inline-flex items-center align-baseline leading-tight text-sm relative bg-blue-100 text-blue-800 border border-blue-200 rounded-md px-1 py-0.5 mx-0.5; | |
| max-width: 200px; | |
| white-space: nowrap; | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| cursor: default; | |
| } | |
| } | |
| .ql-mention { | |
| padding-top: 0px !important; | |
| } | |
| .ql-mention::before { | |
| content: '@'; | |
| font-size: 14px; | |
| } | |
| .rich-editor { | |
| span[mention] { | |
| @apply inline-flex items-center align-baseline leading-tight text-sm relative bg-blue-100 text-blue-800 border border-blue-200 rounded-md px-1 py-0.5 mx-0.5; | |
| max-width: 200px; | |
| white-space: nowrap; | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| cursor: default; | |
| } | |
| } |
| const setContents = (content) => { | ||
| if (!quillInstance) return | ||
|
|
||
| isInternalChange = true | ||
| quillInstance.root.innerHTML = content | ||
| quillInstance.update() | ||
| isInternalChange = false | ||
| } |
There was a problem hiding this comment.
Avoid setting innerHTML directly to prevent XSS vulnerabilities
Assigning content directly to quillInstance.root.innerHTML can introduce XSS vulnerabilities if the content is not sanitized. It's safer to use Quill's clipboard.dangerouslyPasteHTML() method, which properly handles HTML content.
Apply this diff to fix the issue:
const setContents = (content) => {
if (!quillInstance) return
isInternalChange = true
- quillInstance.root.innerHTML = content
- quillInstance.update()
+ quillInstance.clipboard.dangerouslyPasteHTML(content)
isInternalChange = 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 setContents = (content) => { | |
| if (!quillInstance) return | |
| isInternalChange = true | |
| quillInstance.root.innerHTML = content | |
| quillInstance.update() | |
| isInternalChange = false | |
| } | |
| const setContents = (content) => { | |
| if (!quillInstance) return | |
| isInternalChange = true | |
| quillInstance.clipboard.dangerouslyPasteHTML(content) | |
| isInternalChange = false | |
| } |
| quillInstance.on('text-change', (delta, oldContents, source) => { | ||
| if (!isInternalChange) { | ||
| const html = quillInstance.root.innerHTML | ||
| emit('text-change', { delta, oldContents, source }) | ||
| emit('update:modelValue', html) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Correct the parameter name in the 'text-change' event handler
The parameter oldContents should be renamed to oldDelta to match Quill's event signature for text-change. This ensures accurate data handling within the event.
Apply this diff to fix the parameter name:
quillInstance.on('text-change', (delta, oldContents, source) => {
+ quillInstance.on('text-change', (delta, oldDelta, source) => {
if (!isInternalChange) {
const html = quillInstance.root.innerHTML
- emit('text-change', { delta, oldContents, source })
+ emit('text-change', { delta, oldDelta, source })
emit('update:modelValue', html)
}
})Committable suggestion was skipped due to low confidence.
| onBeforeUnmount(() => { | ||
| if (quillInstance) { | ||
| quillInstance.off('selection-change') | ||
| quillInstance.off('text-change') | ||
| quillInstance.off('editor-change') | ||
| } | ||
| quillInstance = null | ||
| }) |
There was a problem hiding this comment.
Event listeners are not properly removed on unmount
When removing event listeners with quillInstance.off(), you need to provide the same handler functions used in quillInstance.on(). Since the handlers are defined inline, they cannot be referenced for removal, potentially leading to memory leaks.
Refactor the code to store handler references for proper removal:
+let selectionChangeHandler = null
+let textChangeHandler = null
+let editorChangeHandler = null
const initializeQuill = () => {
if (container.value) {
quillInstance = new Quill(container.value, props.options)
+ selectionChangeHandler = (range, oldRange, source) => {
if (!range) {
emit('blur', quillInstance)
} else {
emit('focus', quillInstance)
}
emit('selection-change', { range, oldRange, source })
}
- quillInstance.on('selection-change', (range, oldRange, source) => {
- // handler code
- })
+ quillInstance.on('selection-change', selectionChangeHandler)
+ textChangeHandler = (delta, oldDelta, source) => {
if (!isInternalChange) {
const html = quillInstance.root.innerHTML
emit('text-change', { delta, oldDelta, source })
emit('update:modelValue', html)
}
}
- quillInstance.on('text-change', (delta, oldDelta, source) => {
- // handler code
- })
+ quillInstance.on('text-change', textChangeHandler)
+ editorChangeHandler = (eventName, ...args) => {
emit('editor-change', eventName, ...args)
}
- quillInstance.on('editor-change', (eventName, ...args) => {
- // handler code
- })
+ quillInstance.on('editor-change', editorChangeHandler)
if (props.modelValue) {
setContents(props.modelValue)
}
emit('ready', quillInstance)
}
}
onBeforeUnmount(() => {
if (quillInstance) {
- quillInstance.off('selection-change')
- quillInstance.off('text-change')
- quillInstance.off('editor-change')
+ quillInstance.off('selection-change', selectionChangeHandler)
+ quillInstance.off('text-change', textChangeHandler)
+ quillInstance.off('editor-change', editorChangeHandler)
}
quillInstance = null
})Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
client/components/forms/RichTextAreaInput.client.vue (1)
82-106: Dynamic Quill options configurationThe
quillOptionscomputed property provides a flexible way to configure the Quill editor. It merges default options with user-provided options and adds mention-specific configuration when enabled.Consider extracting the default options into a separate constant to improve readability:
const DEFAULT_QUILL_OPTIONS = { theme: 'snow', modules: { toolbar: [ [{ 'header': 1 }, { 'header': 2 }], ['bold', 'italic', 'underline', 'strike'], ['link'], [{ list: 'ordered' }, { list: 'bullet' }], [{ color: [] }], ] } }; const quillOptions = computed(() => { const mergedOptions = { ...DEFAULT_QUILL_OPTIONS, ...props.editorOptions, modules: { ...DEFAULT_QUILL_OPTIONS.modules, ...props.editorOptions.modules } }; if (props.enableMentions) { mergedOptions.modules.mention = true; if (!mergedOptions.modules.toolbar) { mergedOptions.modules.toolbar = []; } mergedOptions.modules.toolbar.push(['mention']); } return mergedOptions; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- client/components/forms/MentionInput.vue (1 hunks)
- client/components/forms/RichTextAreaInput.client.vue (3 hunks)
- client/composables/useParseMention.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
client/components/forms/RichTextAreaInput.client.vue (8)
7-21: Improved styling and responsiveness with wrapper divThe addition of a wrapper div with dynamic classes and styles enhances the component's flexibility and responsiveness. The use of computed classes based on component state (e.g.,
hasError,disabled) is a good practice for maintaining visual consistency.
51-51: New imports to support editor and mention functionalityThe addition of new imports (Quill, QuillyEditor, MentionDropdown, and registerMentionExtension) supports the enhanced editor functionality and mention feature.
Also applies to: 54-56
57-71: Updated props definitionThe props definition has been updated to include
enableMentionsandmentions. This change provides better control over the mention functionality and allows for external configuration of available mentions.
77-81: Mention extension registration in onMounted hookThe mention extension is now registered in the
onMountedhook, ensuring that it's only initialized when the component is mounted and when mentions are enabled. This is a good practice for performance and conditional feature enabling.
115-115: Use of CSS variable for font sizeThe use of a CSS variable for font size (
var(--font-size)) improves maintainability and allows for easier theming.
148-163: Styles for mentionsThe added styles for mentions provide a clear visual distinction for mentioned items. The use of Tailwind classes for styling is consistent with the project's approach.
As previously mentioned in a past review comment, the
::afterpseudo-element for adding the '@' symbol before mentions should be changed to::beforefor better semantic meaning and consistency with common UI patterns. Please apply the following diff:-.ql-mention::after { +.ql-mention::before { content: '@'; - font-size: 16px; + font-size: 14px; }
41-45: Conditional rendering of MentionDropdown componentThe MentionDropdown component is now conditionally rendered based on the
enableMentionsprop andmentionState. This implementation allows for better control over the mention functionality.To ensure the MentionDropdown component is properly implemented and imported, please run the following script:
#!/bin/bash # Verify MentionDropdown component implementation rg --type vue "export default" ./client/components/forms/components/MentionDropdown.vue
22-31: Replacement of VueEditor with QuillyEditorThe VueEditor component has been replaced with a custom QuillyEditor component. This change likely provides better control over the editor's functionality and appearance.
To ensure the QuillyEditor component is properly implemented and imported, please run the following script:
client/components/forms/MentionInput.vue (2)
1-56: Well-structured and flexible template implementation.The template structure is well-organized and follows Vue.js best practices. The use of slots for labels, help text, and error messages provides great flexibility for customization. The contenteditable div as the main input allows for rich text input including mentions, which is a good approach for this use case.
1-190: Overall assessment: Well-implemented component with room for improvementThe MentionInput component is well-structured and implements complex functionality in a clear and organized manner. It demonstrates good use of Vue.js features and best practices, such as the Composition API, slots for flexibility, and scoped styles.
However, there are a few key areas that need attention:
- Input sanitization to prevent XSS vulnerabilities
- Null checks for selection objects to prevent runtime errors
- Accessibility improvements for the placeholder text
Addressing these issues will significantly enhance the security, robustness, and accessibility of the component. Once these improvements are implemented, this will be a high-quality, reusable component for handling mention inputs.
client/composables/useParseMention.js (3)
3-39: Well-structured implementation ofuseParseMentionThe
useParseMentionfunction is well-designed and effectively processes mentions within the content. UtilizingFormSubmissionFormatterand DOM manipulation provides a clear and maintainable approach to achieve the desired functionality.
12-14: EnsureDOMParseris available in all target environmentsThe use of
DOMParseris appropriate for browser environments. If this code needs to run in non-browser environments (e.g., server-side rendering),DOMParsermay not be available.Run the following script to check for server-side usage of
useParseMention:#!/bin/bash # Description: Search for server-side usage of useParseMention. # Test: Find all server-side JavaScript files that import useParseMention. Expect: No results. rg --type js "import.*useParseMention" server/If the script returns any results, consider adding environment checks or fallbacks.
1-1: Verify the correctness of the import path forFormSubmissionFormatterPlease ensure that the import path
'~/components/forms/components/FormSubmissionFormatter'correctly points to theFormSubmissionFormattermodule. An incorrect path could lead to import errors at runtime.Run the following script to verify the existence of
FormSubmissionFormatter.jsat the specified path:✅ Verification successful
Import path for
FormSubmissionFormatteris correct.The file
FormSubmissionFormatter.jsexists atclient/components/forms/components/FormSubmissionFormatter.js, which matches the import path'~/components/forms/components/FormSubmissionFormatter'.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that FormSubmissionFormatter.js exists at the specified path. # Test: Find the file. Expect: The file path should be displayed. fd 'FormSubmissionFormatter.js' components/forms/components/Length of output: 183
Script:
#!/bin/bash # Description: Verify the existence of FormSubmissionFormatter.js in the codebase. # Test: Search for FormSubmissionFormatter.js across the repository. fd 'FormSubmissionFormatter.js'Length of output: 93
| import MentionDropdown from './components/MentionDropdown.vue' | ||
| const props = defineProps({ | ||
| ...inputProps, | ||
| mentions: { type: Array, default: () => [] }, | ||
| }) | ||
| const emit = defineEmits(['update:modelValue']) | ||
| const { compVal, inputStyle, hasValidation, hasError, inputWrapperProps } = useFormInput(props, { emit }) | ||
| const editableDiv = ref(null) | ||
| const savedRange = ref(null) | ||
| const mentionState = ref({ | ||
| open: false, | ||
| onInsert: (mention) => { | ||
| insertMention(mention) | ||
| mentionState.value.open = false | ||
| }, | ||
| onCancel: () => { | ||
| mentionState.value.open = false | ||
| restoreSelection() | ||
| }, | ||
| }) | ||
| const createMentionSpan = (mention) => { | ||
| const mentionSpan = document.createElement('span') | ||
| mentionSpan.setAttribute('mention-field-id', mention.field.id) | ||
| mentionSpan.setAttribute('mention-field-name', mention.field.name) | ||
| mentionSpan.setAttribute('mention-fallback', mention.fallback || '') | ||
| mentionSpan.setAttribute('contenteditable', 'false') | ||
| mentionSpan.setAttribute('mention', 'true') | ||
| mentionSpan.textContent = mention.field.name.length > 25 ? `${mention.field.name.slice(0, 25)}...` : mention.field.name | ||
| return mentionSpan | ||
| } | ||
| const insertMention = (mention) => { | ||
| const mentionSpan = createMentionSpan(mention) | ||
| restoreSelection() | ||
| const range = window.getSelection().getRangeAt(0) | ||
| // Insert the mention span | ||
| range.insertNode(mentionSpan) | ||
|
|
||
| // Move the cursor after the inserted mention | ||
| range.setStartAfter(mentionSpan) | ||
| range.collapse(true) | ||
| // Apply the new selection | ||
| const selection = window.getSelection() | ||
| selection.removeAllRanges() | ||
| selection.addRange(range) | ||
| // Ensure the editableDiv is focused | ||
| editableDiv.value.focus() | ||
| updateCompVal() | ||
| } | ||
| const openMentionDropdown = () => { | ||
| saveSelection() | ||
| if (!savedRange.value) { | ||
| // If no previous selection, move cursor to the end | ||
| const range = document.createRange() | ||
| range.selectNodeContents(editableDiv.value) | ||
| range.collapse(false) | ||
| const selection = window.getSelection() | ||
| selection.removeAllRanges() | ||
| selection.addRange(range) | ||
| savedRange.value = range | ||
| } | ||
| mentionState.value.open = true | ||
| } | ||
| const saveSelection = () => { | ||
| const selection = window.getSelection() | ||
| if (selection.rangeCount > 0) { | ||
| savedRange.value = selection.getRangeAt(0) | ||
| } | ||
| } | ||
| const restoreSelection = () => { | ||
| if (savedRange.value) { | ||
| const selection = window.getSelection() | ||
| selection.removeAllRanges() | ||
| selection.addRange(savedRange.value) | ||
| editableDiv.value.focus() | ||
| } | ||
| } | ||
| const updateCompVal = () => { | ||
| compVal.value = editableDiv.value.innerHTML | ||
| } | ||
| const onInput = () => { | ||
| updateCompVal() | ||
| } | ||
| onMounted(() => { | ||
| if (compVal.value) { | ||
| editableDiv.value.innerHTML = compVal.value | ||
| } | ||
| }) | ||
| watch(compVal, (newVal) => { | ||
| if (editableDiv.value && editableDiv.value.innerHTML !== newVal) { | ||
| editableDiv.value.innerHTML = newVal | ||
| } | ||
| }) | ||
| defineExpose({ | ||
| editableDiv, | ||
| compVal, | ||
| mentionState, | ||
| openMentionDropdown, | ||
| onInput, | ||
| }) | ||
| </script> |
There was a problem hiding this comment.
Comprehensive mention handling, but with room for improvement
The script section demonstrates good use of the Composition API and composables. However, there are a few areas that need attention:
-
Input sanitization: The component doesn't sanitize user input, which could lead to XSS vulnerabilities. Consider using a library like DOMPurify to sanitize the HTML content.
-
Null checks: Some methods like
insertMentionandrestoreSelectiondon't have null checks for selection objects, which could lead to runtime errors. -
Direct innerHTML assignment: The
updateCompValmethod directly assigns innerHTML, which could be a security risk. Consider using safer alternatives or ensuring proper sanitization.
To address these issues, consider implementing the following changes:
- Add input sanitization:
import DOMPurify from 'dompurify'
const updateCompVal = () => {
compVal.value = DOMPurify.sanitize(editableDiv.value.innerHTML)
}
onMounted(() => {
if (compVal.value) {
editableDiv.value.innerHTML = DOMPurify.sanitize(compVal.value)
}
})
watch(compVal, (newVal) => {
if (editableDiv.value && editableDiv.value.innerHTML !== newVal) {
editableDiv.value.innerHTML = DOMPurify.sanitize(newVal)
}
})- Add null checks for selection objects:
const insertMention = (mention) => {
const mentionSpan = createMentionSpan(mention)
restoreSelection()
const selection = window.getSelection()
if (!selection || selection.rangeCount === 0) return
const range = selection.getRangeAt(0)
// ... rest of the method
}
const restoreSelection = () => {
if (savedRange.value) {
const selection = window.getSelection()
if (!selection) return
selection.removeAllRanges()
selection.addRange(savedRange.value)
editableDiv.value.focus()
}
}These changes will improve the security and robustness of the component.
| <style scoped> | ||
| .mention-input { | ||
| min-height: 1.5rem; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| } | ||
| .mention-input:empty::before { | ||
| content: attr(placeholder); | ||
| color: #9ca3af; | ||
| } | ||
| .mention-input span[mention] { | ||
| max-width: 150px; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| background-color: #dbeafe; | ||
| color: #1e40af; | ||
| border: 1px solid #bfdbfe; | ||
| border-radius: 0.25rem; | ||
| padding: 0 0.25rem; | ||
| font-size: 0.875rem; | ||
| line-height: 1.25rem; | ||
| position: relative; | ||
| vertical-align: baseline; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving placeholder accessibility
The styles are well-structured and scoped to the component, which is good practice. However, the use of a pseudo-element for the placeholder text might not be accessible to screen readers.
Consider using the aria-placeholder attribute instead of the pseudo-element for better accessibility. Here's how you can modify the template and styles:
- Update the template:
<div
ref="editableDiv"
:contenteditable="!disabled"
class="mention-input"
:style="inputStyle"
:aria-placeholder="placeholder"
@input="onInput"
<!-- ... other attributes ... -->
/>- Update the styles:
.mention-input:empty::before {
content: attr(aria-placeholder);
color: #9ca3af;
}This change will make the placeholder text accessible to screen readers while maintaining the visual appearance.
| if (value) { | ||
| if (Array.isArray(value)) { | ||
| element.textContent = value.join(', ') | ||
| } else { | ||
| element.textContent = value | ||
| } | ||
| } else if (fallback) { | ||
| element.textContent = fallback | ||
| } else { | ||
| element.remove() | ||
| } |
There was a problem hiding this comment.
Handle cases where value is an empty string or zero
The current condition if (value) will treat 0 or an empty string '' as falsy, potentially skipping valid values. This could lead to unintended behavior where legitimate data is ignored.
Consider updating the condition to explicitly check for null or undefined:
- if (value) {
+ if (value !== null && value !== undefined) {This ensures that values like 0 or '' are correctly processed.
📝 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.
| if (value) { | |
| if (Array.isArray(value)) { | |
| element.textContent = value.join(', ') | |
| } else { | |
| element.textContent = value | |
| } | |
| } else if (fallback) { | |
| element.textContent = fallback | |
| } else { | |
| element.remove() | |
| } | |
| if (value !== null && value !== undefined) { | |
| if (Array.isArray(value)) { | |
| element.textContent = value.join(', ') | |
| } else { | |
| element.textContent = value | |
| } | |
| } else if (fallback) { | |
| element.textContent = fallback | |
| } else { | |
| element.remove() | |
| } |
| mentionElements.forEach(element => { | ||
| const fieldId = element.getAttribute('mention-field-id') | ||
| const fallback = element.getAttribute('mention-fallback') | ||
| const value = formattedData[fieldId] | ||
|
|
||
| if (value) { | ||
| if (Array.isArray(value)) { | ||
| element.textContent = value.join(', ') | ||
| } else { | ||
| element.textContent = value | ||
| } | ||
| } else if (fallback) { | ||
| element.textContent = fallback | ||
| } else { | ||
| element.remove() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add checks for undefined fieldId
To prevent potential errors, add a check to ensure that fieldId is defined before accessing formattedData[fieldId]. Accessing an undefined key could lead to unintended results.
Consider modifying the code as follows:
mentionElements.forEach(element => {
const fieldId = element.getAttribute('mention-field-id')
const fallback = element.getAttribute('mention-fallback')
+ if (fieldId) {
const value = formattedData[fieldId]
- if (value !== null && value !== undefined) {
+ if (value !== null && value !== undefined) {
if (Array.isArray(value)) {
element.textContent = value.join(', ')
} else {
element.textContent = value
}
} else if (fallback) {
element.textContent = fallback
} else {
element.remove()
}
+ } else if (fallback) {
+ element.textContent = fallback
+ } else {
+ element.remove()
+ }
})This ensures that you only attempt to access formattedData when fieldId is valid.
📝 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.
| mentionElements.forEach(element => { | |
| const fieldId = element.getAttribute('mention-field-id') | |
| const fallback = element.getAttribute('mention-fallback') | |
| const value = formattedData[fieldId] | |
| if (value) { | |
| if (Array.isArray(value)) { | |
| element.textContent = value.join(', ') | |
| } else { | |
| element.textContent = value | |
| } | |
| } else if (fallback) { | |
| element.textContent = fallback | |
| } else { | |
| element.remove() | |
| } | |
| }) | |
| mentionElements.forEach(element => { | |
| const fieldId = element.getAttribute('mention-field-id') | |
| const fallback = element.getAttribute('mention-fallback') | |
| if (fieldId) { | |
| const value = formattedData[fieldId] | |
| if (value !== null && value !== undefined) { | |
| if (Array.isArray(value)) { | |
| element.textContent = value.join(', ') | |
| } else { | |
| element.textContent = value | |
| } | |
| } else if (fallback) { | |
| element.textContent = fallback | |
| } else { | |
| element.remove() | |
| } | |
| } else if (fallback) { | |
| element.textContent = fallback | |
| } else { | |
| element.remove() | |
| } | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- client/components/open/forms/OpenCompleteForm.vue (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
client/components/open/forms/OpenCompleteForm.vue (2)
143-149: Excellent improvement in handling submitted text display!The replacement of
v-htmlwith aTextBlockcomponent is a significant enhancement:
- It improves security by potentially mitigating XSS vulnerabilities associated with
v-html.- It allows for more structured and controlled rendering of the submitted text.
- The new props (
mentions-allowed,form, andform-data) suggest advanced functionality for handling mentions and form data within the submitted text.This change aligns well with best practices for secure and flexible content rendering.
229-230: Good addition ofsubmittedDatapropertyThe new
submittedDataproperty aligns well with the changes in the template section, allowing for better management of submitted form data. Initializing it tonullis a good practice for clarity.
| submissionId: false, | ||
| submittedData: null |
There was a problem hiding this comment.
Consider resetting submittedData in the restart method
While the addition of submittedData is good, it's important to ensure it's properly reset when the form is restarted. This aligns with the previous review comment that's still relevant.
Consider updating the restart method to reset submittedData:
restart () {
this.submitted = false
+ this.submittedData = null
this.$emit('restarted', true)
}This change will prevent potential data leakage between form submissions.
Committable suggestion was skipped due to low confidence.
|
Closing in favor of #595 |
Summary by CodeRabbit
Release Notes
New Features
MentionInput,MentionDropdown,FormSubmissionFormatter,TextBlock, andOpenFormsHtmlDefinition.SubmissionConfirmationIntegrationfor better user interface and input handling.Improvements
QuillyEditorcomponent, adding support for mentions.Bug Fixes