Phone validate#1515
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| var fieldValue = getPreChatInputValue(field); | ||
| if (field.hasAttribute('required') && !fieldValue) { | ||
| return getLeadCollectionLabel('commonErrorMsg', ''); |
There was a problem hiding this comment.
Bug: If the labels object in appOptions is completely replaced with an incomplete set, form validation can be silently bypassed because required error messages are missing.
Severity: LOW
Suggested Fix
Ensure validation logic is independent of the presence of a label string. The validation function should return a clear status (e.g., boolean), and the UI logic should handle displaying an error message separately. Alternatively, fall back to a hardcoded, generic error message if a specific label is not found during a validation failure.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: webplugin/js/app/prechat/km-prechat.js#L599-L601
Potential issue: The validation logic for the pre-chat form relies on error message
labels being present in the configuration. If a user provides a severely misconfigured
`appOptions` object that completely replaces the default `labels` object with an
incomplete one (missing keys like `commonErrorMsg`), the validation for fields like
phone number will fail silently. This is because the code checks for the presence of the
error message string before displaying it, and if the string is empty, no error is
shown, allowing the user to proceed with invalid data. While this is an edge case, as
the system is designed to deep-merge configurations, it is technically possible.
Also affects:
webplugin/js/app/prechat/km-prechat.js:604~606webplugin/js/app/prechat/km-prechat.js:625~627webplugin/js/app/prechat/km-prechat.js:643~645
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
It will take default error msg
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCentralizes pre-chat validation: adds per-field regex attributes, shared validation helpers and a new target.validatePreChatForm(), updates inline event wiring and submit gating, and removes legacy phone keydown attach/detach calls. ChangesPre-Chat Validation Refactoring
Sequence DiagramsequenceDiagram
actor User
participant Sidebox as Sidebox Submit
participant PreChat as PreChat Module
participant DOM as Form Elements & Error Display
User->>Sidebox: Submit form
Sidebox->>DOM: Read error node visibility/text
Sidebox->>PreChat: Call validatePreChatForm()
PreChat->>DOM: Iterate visible form controls
loop For each field
PreChat->>DOM: Read field value & attrs
PreChat->>PreChat: Validate (required/email/regex/phone)
alt Validation fails
PreChat->>DOM: Set error message on error display
PreChat->>PreChat: Return false
end
end
PreChat-->>Sidebox: Validation result
alt Validation passed
Sidebox->>DOM: Allow submission
else Validation failed
Sidebox->>Sidebox: Return false (prevent submission)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webplugin/js/app/prechat/km-prechat.js (1)
611-623: Silent failure on invalid regex could allow unvalidated input.When
new RegExp(validationRegex)throws (line 613-615),regexis set tonulland the validation silently passes (line 617 requiresregexto be truthy). This means a misconfigured regex pattern would effectively disable validation for that field without any indication.Consider logging a warning to help administrators identify configuration issues:
Suggested improvement
try { regex = new RegExp(validationRegex); } catch (e) { + console.warn('Invalid validation regex for field:', field.id, e.message); regex = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/prechat/km-prechat.js` around lines 611 - 623, The current try/catch around new RegExp(validationRegex) swallows RegExp syntax errors and allows validation to silently pass; update the catch to log a warning that includes the offending validationRegex and the caught error (e.g., using console.warn or the existing logger) and then treat the field as invalid (return the field's data-validation-error-text or getLeadCollectionLabel('commonErrorMsg','')) instead of setting regex to null and skipping validation; locate the logic around new RegExp(validationRegex), the regex variable, fieldValue, and the existing return that uses getLeadCollectionLabel to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webplugin/js/app/mck-sidebox-1.0.js`:
- Around line 4995-4998: The branch in MckMessageService incorrectly checks
_this.validatePreChatForm (which is not set there); update the code so the
actual validator attached by mckInit is invoked: either (a) call the validator
directly from the KMPreChat attachment (e.g. use KMPreChat.validatePreChatForm()
or the function reference created by KMPreChat.attach(...) inside this branch)
or (b) during mckInit assign the validator onto the service (e.g. set
MckMessageService.validatePreChatForm = KMPreChat.validatePreChatForm) and then
use that service property; specifically modify the conditional that references
_this.validatePreChatForm and ensure it invokes the validator function provided
by KMPreChat.attach(...) (validatePreChatForm) so pre-chat validation actually
runs before submitting.
---
Nitpick comments:
In `@webplugin/js/app/prechat/km-prechat.js`:
- Around line 611-623: The current try/catch around new RegExp(validationRegex)
swallows RegExp syntax errors and allows validation to silently pass; update the
catch to log a warning that includes the offending validationRegex and the
caught error (e.g., using console.warn or the existing logger) and then treat
the field as invalid (return the field's data-validation-error-text or
getLeadCollectionLabel('commonErrorMsg','')) instead of setting regex to null
and skipping validation; locate the logic around new RegExp(validationRegex),
the regex variable, fieldValue, and the existing return that uses
getLeadCollectionLabel to implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0859aab-5e7f-4887-867e-02f605fff321
📒 Files selected for processing (2)
webplugin/js/app/mck-sidebox-1.0.jswebplugin/js/app/prechat/km-prechat.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webplugin/js/app/prechat/km-prechat.js (1)
746-771: ⚡ Quick winMake inline validation listener binding idempotent.
target.addPreChatInlineValidationadds anonymousinput/blur/clicklisteners every call. If this initializer runs again, handlers stack and validation fires multiple times per event.Proposed fix
target.addPreChatInlineValidation = function () { var form = document.getElementById('km-form-chat-login'); var submitBtn = document.getElementById('km-submit-chat-login'); + if (form && form.getAttribute('data-prechat-validation-bound') === 'true') { + return; + } + if (form) { + form.setAttribute('data-prechat-validation-bound', 'true'); + } var formSubmitted = false; var formFields = form ? form.querySelectorAll('.km-form-control') : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webplugin/js/app/prechat/km-prechat.js` around lines 746 - 771, The addPreChatInlineValidation function currently attaches anonymous input/blur/click handlers each time it's called, causing duplicate handlers; to fix, make binding idempotent by detecting and skipping if already bound (e.g., check a flag or element dataset/property) or attach named handler functions and remove existing listeners before adding; update target.addPreChatInlineValidation to use stable handler functions (referencing target.validatePreChatForm) and set a marker on the form or submitBtn (e.g., form.__kmInlineValidationBound or form.dataset.kmBound) after initial binding so subsequent calls do nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webplugin/js/app/prechat/km-prechat.js`:
- Around line 701-708: The current catch swallows invalid regex and lets
validation pass; instead when RegExp(validationRegex) throws, mark the field as
invalid (fail closed) by setting a failure flag or forcing the regex to never
match and add a validation error for that field; specifically, inside the catch
for new RegExp(validationRegex) update the validation outcome for field (use the
same field, fieldValue, validationRegex, regex symbols) to produce a fallback
error message and log the parsing exception so the invalid config does not
silently disable validation.
---
Nitpick comments:
In `@webplugin/js/app/prechat/km-prechat.js`:
- Around line 746-771: The addPreChatInlineValidation function currently
attaches anonymous input/blur/click handlers each time it's called, causing
duplicate handlers; to fix, make binding idempotent by detecting and skipping if
already bound (e.g., check a flag or element dataset/property) or attach named
handler functions and remove existing listeners before adding; update
target.addPreChatInlineValidation to use stable handler functions (referencing
target.validatePreChatForm) and set a marker on the form or submitBtn (e.g.,
form.__kmInlineValidationBound or form.dataset.kmBound) after initial binding so
subsequent calls do nothing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dac262e-d6ac-4fce-aee6-81fb6bf36208
📒 Files selected for processing (2)
webplugin/js/app/mck-sidebox-1.0.jswebplugin/js/app/prechat/km-prechat.js
🚧 Files skipped from review as they are similar to previous changes (1)
- webplugin/js/app/mck-sidebox-1.0.js
ayush-kommunicate
left a comment
ayush-kommunicate
left a comment
There was a problem hiding this comment.
Check AI comment once any change required or not.
2 participants
What do you want to achieve?
PR Checklist
How was the code tested?
What new thing you came across while writing this code?
In case you fixed a bug then please describe the root cause of it?
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit
New Features
Bug Fixes