TDH-3129-new Add configuration to allow certain extensions only#1440
TDH-3129-new Add configuration to allow certain extensions only#1440kandpal025 merged 3 commits intodevelopmentfrom
Conversation
WalkthroughCentralized file-extension error handling in the upload flow by adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. 🔧 Biome (2.1.2)webplugin/js/app/mck-sidebox-1.0.jsThe --json option is unstable/experimental and its output might change between patches/minor releases. ... [truncated 99999690 characters] ... This is a bug in Biome, not an error in your code, and we would appreciate it if you could report it to https://github.com/biomejs/biome/issues/ along with the following information to help us fixing the issue. When opening the issue, please provide a minimal reproduction, or identify and share the file/code that triggers it. Without a way to reproduce the error, the error can't be fixed: Source Location: crates/biome_console/src/lib.rs:151:14 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: 0
🧹 Nitpick comments (3)
webplugin/js/app/mck-sidebox-1.0.js (3)
13463-13478: Good centralized error display with timeout management.The implementation correctly clears any pending timeout before setting a new one, preventing message overlap. The 5-second auto-hide provides good UX.
Note: AI summary incorrectly states the parameter is
messageKey, but it's actuallyerrorMessage(line 13463).Minor consideration: If the containing component/service can be destroyed while a timeout is pending, consider adding cleanup logic to prevent memory leaks. However, given the isolated scope mentioned in learnings, this may not be a concern.
13878-13911: Error handling works but has maintainability concerns.The logic correctly handles FILE_TYPE_NOT_ALLOWED errors with proper UI cleanup and message removal.
Code duplication: The error extraction logic (lines 13888-13894) is duplicated from lines 13718-13724 in hunk 4. Consider extracting this into a helper function:
♻️ Refactor to eliminate duplication
var extractFileTypeErrorMessage = function(responseJson) { return (responseJson.errorResponse && responseJson.errorResponse[0] && (responseJson.errorResponse[0].description || responseJson.errorResponse[0].displayMessage)) || (responseJson && responseJson.errorMessage) || 'File type is not allowed.'; };Then replace the duplicated code with:
- var errorMsg = - (responseJson.errorResponse && - responseJson.errorResponse[0] && - (responseJson.errorResponse[0].description || - responseJson.errorResponse[0].displayMessage)) || - (responseJson && responseJson.errorMessage) || - 'File type is not allowed.'; + var errorMsg = extractFileTypeErrorMessage(responseJson);Naming clarity: Lines 13895 and 13897 call two different functions both named
showFileExtensionError:
- Line 13895: local function that displays error message to user
- Line 13897:
_this.showFileExtensionError(messagePxy.key)which resets UI state (defined in hunk 6)While both calls appear intentional, the identical naming is confusing. Consider renaming the service method to something like
resetFileUploadUIfor clarity.
14156-14161: Function naming creates confusion.The logic correctly resets the UI state for a message by showing the upload icon, hiding the cancel icon and progress bar.
Critical naming issue: This method
_this.showFileExtensionError(messageKey)has the same name as the local function at line 13463, but serves a completely different purpose:
- Local function (line 13463): Displays an error message string to the user with a 5-second timeout
- Service method (line 14156): Resets UI state (shows/hides icons) for a specific message key
This creates significant confusion, especially evident in hunk 5 where both are called sequentially (lines 13895, 13897).
Recommendation: Rename this service method to reflect its actual purpose:
♻️ Suggested rename
- _this.showFileExtensionError = function (messageKey) { + _this.resetFileUploadUI = function (messageKey) { kommunicateCommons.show('.km-attachment-upload-icon-' + messageKey); kommunicateCommons.hide('.km-attachment-cancel-icon-' + messageKey); kommunicateCommons.show('.mck-timestamp-' + messageKey); kommunicateCommons.hide('.km-attachment-progress-bar-wrapper-' + messageKey); };Then update the call site at line 13897:
- _this.showFileExtensionError(messagePxy.key); + _this.resetFileUploadUI(messagePxy.key);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
Repo: Kommunicate-io/Kommunicate-Web-SDK PR: 1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (3)
webplugin/js/app/mck-sidebox-1.0.js (3)
13448-13448: LGTM - Error element reference added.The
$mck_msg_errorvariable is correctly initialized and will be used by the new error handling function.
13651-13658: LGTM - Consistent error handling added.The code properly displays upload errors using the centralized error function and cleans up failed messages from the UI.
13708-13730: Robust error handling for blocked file types.The implementation correctly:
- Handles multiple response formats (errorResponse array and direct errorCode)
- Extracts error messages with a sensible fallback chain
- Performs UI cleanup (re-enables buttons, triggers file removal)
- Returns early to prevent further processing
Observation: This handler only calls the local
showFileExtensionError(errorMsg)function, whereas hunk 5 (lines 13895-13897) calls both the local function and_this.showFileExtensionError(messagePxy.key). Verify this difference is intentional based on the upload context.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (messagePxy) { | ||
| mckMessageLayout.removedDeletedMessage( | ||
| messagePxy.key, | ||
| messagePxy.groupId, | ||
| true | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The code is correct for its intended use case. The if (messagePxy) check is appropriate because cleanup is only needed when a message was actually created. No fix needed unless you're seeing actual orphaned UI elements in production.
| return; | ||
| } | ||
| if ( | ||
| (responseJson && |
There was a problem hiding this comment.
duplicate code checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webplugin/js/app/mck-sidebox-1.0.js (2)
13889-13891: Confusing naming: local function and instance method share the same name but have different purposes.Line 13889 calls the local
showFileExtensionError(errorMsg)which displays an error message, while line 13891 calls_this.showFileExtensionError(messagePxy.key)which updates UI elements (icons, progress bars). This is confusing and error-prone.Consider renaming one of them:
- Rename the local function to
displayFileExtensionErrorMessage(errorMessage)- Or rename the instance method to
resetAttachmentUIOnError(messageKey)or similar
13882-13888: Duplicate error message extraction logic.This error message extraction logic is identical to lines 13715-13721 in the
uploadFilehandler. Consider extracting into a helper function.♻️ Proposed refactor
Add a helper function near the top of the file service:
var extractFileErrorMessage = function (responseJson) { return (responseJson.errorResponse && responseJson.errorResponse[0] && (responseJson.errorResponse[0].description || responseJson.errorResponse[0].displayMessage)) || (responseJson && responseJson.errorMessage) || 'File type is not allowed.'; };Then use it in both handlers:
-var errorMsg = - (responseJson.errorResponse && - responseJson.errorResponse[0] && - (responseJson.errorResponse[0].description || - responseJson.errorResponse[0].displayMessage)) || - (responseJson && responseJson.errorMessage) || - 'File type is not allowed.'; +var errorMsg = extractFileErrorMessage(responseJson);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
Repo: Kommunicate-io/Kommunicate-Web-SDK PR: 1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (4)
webplugin/js/app/mck-sidebox-1.0.js (4)
13462-13478: LGTM!The timeout management is properly implemented - clearing any existing timeout before setting a new one prevents stacking/duplicate hide operations.
13648-13659: LGTM!Error display and message cleanup are handled correctly.
13708-13727: LGTM!The 403 error handling correctly extracts the error message with appropriate fallbacks and properly cleans up the UI state.
14150-14155: LGTM!The method follows the existing pattern (similar to
showMaliciousFileError) and correctly resets the attachment UI state. As noted earlier, consider a more distinct name to avoid confusion with the localshowFileExtensionErrorfunction.
| return; | ||
| } | ||
| if ( | ||
| this.status === 403 && |
There was a problem hiding this comment.
still duplicate checks
There was a problem hiding this comment.
can you check now ?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webplugin/js/app/mck-sidebox-1.0.js (1)
13480-13526: Consider consolidating the duplicatemessagePxychecks.The function handles error cleanup well, but there are two separate
if (messagePxy)blocks (lines 13503-13505 and 13516-13522) that could be consolidated into a single block for clarity.♻️ Suggested consolidation
showFileExtensionError(errorMsg); - if (messagePxy) { - _this.showFileExtensionError(messagePxy.key); - } if ($file_remove) { $file_remove.attr('disabled', false); $file_remove.trigger('click'); } if ($mck_file_upload) { $mck_file_upload.attr('disabled', false); } if ($mck_msg_sbmt) { $mck_msg_sbmt.attr('disabled', false); } if (messagePxy) { + _this.showFileExtensionError(messagePxy.key); mckMessageLayout.removedDeletedMessage( messagePxy.key, messagePxy.groupId, true ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
Repo: Kommunicate-io/Kommunicate-Web-SDK PR: 1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (6)
webplugin/js/app/mck-sidebox-1.0.js (6)
13448-13448: LGTM!The DOM element reference follows the existing pattern used for other UI elements in this file.
13462-13478: Well-structured timeout handling.The timeout management correctly clears any existing timeout before setting a new one, preventing stacked timeouts. The 5-second auto-hide duration is reasonable for user notification.
13698-13707: LGTM!The error handling correctly notifies the user and cleans up the pending message for validation failures before the upload attempt.
13754-13767: LGTM!The integration correctly delegates error handling and returns early to prevent further processing when a file extension error is detected.
13915-13926: LGTM!Consistent integration of the centralized error handler in the AWS upload path.
14170-14176: No refactoring needed—methods have different responsibilities despite similar patterns.While
showFileExtensionErrorandshowMaliciousFileErrorshare the first two operations (showing the upload icon and hiding the cancel icon), they diverge in their subsequent actions.showMaliciousFileErrorshows an additional error element, whileshowFileExtensionErrorhides the progress bar. Extracting common logic would add unnecessary parameters and reduce clarity. The current implementation is maintainable as-is.
3 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
✏️ Tip: You can customize this high-level summary in your review settings.