Clean up empty HTML and help text in form inputs #700
Conversation
WalkthroughThe pull request updates both backend and frontend components to sanitize input data. In the backend, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestHandler as UserFormRequest
participant Validator
Client->>RequestHandler: Submit Request
RequestHandler->>RequestHandler: prepareForValidation()
RequestHandler->>Validator: Forward sanitized data
sequenceDiagram
participant User
participant Component as RichTextAreaInput
participant Watcher
User->>Component: Input HTML content
Component->>Watcher: Watch compVal change
Watcher->>Component: Strip HTML & check if empty
alt Content is empty
Component->>Component: Set compVal to null
else Content present
Component->>Component: Retain current value
end
Component->>User: Update display
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/components/forms/RichTextAreaInput.client.vue (1)
101-101: Consider using DOMParser for more reliable HTML parsing.The current regex pattern
/<[^>]*>/gmight miss some edge cases. Using DOMParser would be more reliable for stripping HTML.Apply this diff to improve HTML parsing:
- if (val && val.replace(/<[^>]*>/g, '').trim() === '') { + if (val) { + const doc = new DOMParser().parseFromString(val, 'text/html') + if (doc.body.textContent.trim() === '') {api/app/Http/Requests/UserFormRequest.php (1)
23-23: Consider using DOMDocument for more reliable HTML parsing.Similar to the frontend change, using
strip_tags()might miss some edge cases. Consider using DOMDocument for more reliable HTML parsing.Apply this diff to improve HTML parsing:
- if (isset($property['help']) && strip_tags($property['help']) === '') { + if (isset($property['help']) && is_string($property['help'])) { + $dom = new \DOMDocument(); + $dom->loadHTML($property['help'], LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD); + if (trim($dom->textContent) === '') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/app/Http/Requests/UserFormRequest.php(1 hunks)client/components/forms/RichTextAreaInput.client.vue(1 hunks)
| watch(compVal, (val) => { | ||
| if (val && val.replace(/<[^>]*>/g, '').trim() === '') { | ||
| compVal.value = null | ||
| } | ||
| }, { immediate: true }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit update event instead of directly modifying the value.
The watcher directly modifies compVal.value which breaks the one-way data flow pattern in Vue. Instead, emit an update event to let the parent component handle the change.
Apply this diff to fix the implementation:
watch(compVal, (val) => {
if (val && val.replace(/<[^>]*>/g, '').trim() === '') {
- compVal.value = null
+ emit('update:modelValue', null)
}
}, { immediate: true })📝 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.
| watch(compVal, (val) => { | |
| if (val && val.replace(/<[^>]*>/g, '').trim() === '') { | |
| compVal.value = null | |
| } | |
| }, { immediate: true }) | |
| watch(compVal, (val) => { | |
| if (val && val.replace(/<[^>]*>/g, '').trim() === '') { | |
| emit('update:modelValue', null) | |
| } | |
| }, { immediate: true }) |
| foreach ($data['properties'] as &$property) { | ||
| if (isset($property['help']) && strip_tags($property['help']) === '') { // Remove help if it's empty | ||
| $property['help'] = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type checking and avoid reference modification.
The current implementation has two issues:
- No type checking for
property['help'] - Using references (
&$property) for array modification which can lead to unexpected behavior
Apply this diff to improve the implementation:
- foreach ($data['properties'] as &$property) {
- if (isset($property['help']) && strip_tags($property['help']) === '') { // Remove help if it's empty
- $property['help'] = null;
- }
+ $data['properties'] = array_map(function ($property) {
+ if (isset($property['help']) && is_string($property['help']) && strip_tags($property['help']) === '') {
+ $property['help'] = null;
}
- }
+ return $property;
+ }, $data['properties']);📝 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.
| foreach ($data['properties'] as &$property) { | |
| if (isset($property['help']) && strip_tags($property['help']) === '') { // Remove help if it's empty | |
| $property['help'] = null; | |
| } | |
| } | |
| $data['properties'] = array_map(function ($property) { | |
| if (isset($property['help']) && is_string($property['help']) && strip_tags($property['help']) === '') { | |
| $property['help'] = null; | |
| } | |
| return $property; | |
| }, $data['properties']); |
Summary by CodeRabbit