Develop #364
Develop #364
Conversation
Better handling of search params
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure 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: 1
🧹 Nitpick comments (4)
CHANGELOG.md (1)
4-5: Improve clarity in the changelog entry.Consider refining the wording to use plural “parameters” for consistency:
- - Added search parameter ctActionMode to handle all url query parameter + - Added search parameter ctActionMode to handle all URL query parameterssrc/modules/visualBuilder/pageBuilder.js (1)
46-46: Complete the TODO for secure messaging.A guarding mechanism to verify message origins helps avoid security issues like clickjacking or malicious postMessages. Let me know if you need help drafting it.
clevertap.js (2)
11133-11137: Add inline documentation for the new constants.A brief description for each of these action modes (BUILDER, PREVIEW, SDK_CHECK) would enhance maintainability and make the code self-explanatory.
16369-16369: Centralize SDK version definition.Hardcoding the version here and in other places can lead to discrepancies. Consider deduplicating by referencing a single version variable (e.g., from package.json) across all references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.jsyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)clevertap.js(7 hunks)package.json(1 hunks)src/clevertap.js(2 hunks)src/modules/visualBuilder/builder_constants.js(1 hunks)src/modules/visualBuilder/pageBuilder.js(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (9)
src/modules/visualBuilder/builder_constants.js (1)
8-12: Centralize query parameters for maintainability.Defining a dedicated object for your visual editor query parameters is a neat approach. This ensures a single source of truth for references to builder-related mode checks and prevents typos.
src/clevertap.js (2)
46-46: Importing handleActionMode looks appropriate.The addition of
handleActionModealongsideaddAntiFlickeraligns with the refactoring inpageBuilder.js. No issues spotted here.
659-659:❓ Verification inconclusive
Verify that checkBuilder references are fully removed.
Ensure there are no leftover references to the old
checkBuildermethod in the project.
🏁 Script executed:
#!/bin/bash # Searching for any legacy references to checkBuilder rg "checkBuilder" -A 5Length of output: 22
Action Required: Verify Complete Removal of 'checkBuilder' References
Based on our recent searches using ripgrep, there are no detected occurrences of "checkBuilder" in the codebase. The code snippet in
src/clevertap.jsnow correctly callshandleActionModeon line 659, suggesting that the legacycheckBuildermethod has been replaced or removed.However, given that the search output was empty (which could be due to the command’s limitations or missing indirect references), please perform a manual verification to ensure that no dynamic or indirect usages of
checkBuilderremain.src/modules/visualBuilder/pageBuilder.js (2)
1-44: Refactored function improves readability.Renaming
checkBuildertohandleActionModeand leveraging a switch statement for each mode is clearer and more maintainable. This helps ensure each action mode is handled consistently.
168-173: Removing ctActionMode in preview ensures cleaner URLs.Stripping out
ctActionModeavoids polluting the preview mode with development query params. This is a reasonable approach for end-user clarity.clevertap.js (4)
11390-11396: Verify downstream impact of stripping out ‘ctActionMode’.Removing the
ctActionModeparameter upon entering preview mode might confuse users who rely on that query for debugging or direct linking. Ensure that removing it aligns with the expected user flow and does not hinder future debugging steps.
11481-11481: Double-check URL matching logic.Comparing
d.urlwith the modifiedurlmight fail if they differ only by trailing slashes or protocol casing. Consider normalizing both strings or using a more robust comparison if partial matches are valid.
14623-14623: Version bump looks good.Thanks for keeping the library version up to date.
16182-16182: EnsurehandleActionModeis free of window references in non-browser contexts.If the library can run in server-side rendering (SSR) setups or Web Workers, referencing
windowmay result in runtime errors.
| const handleActionMode = (logger, accountId) => { | ||
| const searchParams = new URLSearchParams(window.location.search); | ||
| const ctType = searchParams.get('ctActionMode'); | ||
|
|
||
| if (search === '?ctBuilder') { | ||
| // open in visual builder mode | ||
| logger.debug('open in visual builder mode'); | ||
| window.addEventListener('message', handleMessageEvent, false); | ||
| if (ctType) { | ||
| const parentWindow = window.opener; | ||
|
|
||
| if (parentWindow) { | ||
| parentWindow.postMessage({ | ||
| message: 'builder', | ||
| originUrl: window.location.href | ||
| }, '*'); | ||
| } | ||
| switch (ctType) { | ||
| case WVE_QUERY_PARAMS.BUILDER: | ||
| logger.debug('open in visual builder mode'); | ||
| window.addEventListener('message', handleMessageEvent, false); | ||
|
|
||
| return; | ||
| } | ||
| if (parentWindow) { | ||
| parentWindow.postMessage({ | ||
| message: 'builder', | ||
| originUrl: window.location.href | ||
| }, '*'); | ||
| } | ||
|
|
||
| if (search === '?ctBuilderPreview') { | ||
| window.addEventListener('message', handleMessageEvent, false); | ||
| break; | ||
|
|
||
| if (parentWindow) { | ||
| parentWindow.postMessage({ | ||
| message: 'preview', | ||
| originUrl: window.location.href | ||
| }, '*'); | ||
| } | ||
| } | ||
| case WVE_QUERY_PARAMS.PREVIEW: | ||
| logger.debug('preview of visual editor'); | ||
| window.addEventListener('message', handleMessageEvent, false); | ||
|
|
||
| if (parentWindow) { | ||
| parentWindow.postMessage({ | ||
| message: 'preview', | ||
| originUrl: window.location.href | ||
| }, '*'); | ||
| } | ||
|
|
||
| if (search === '?ctBuilderSDKCheck') { | ||
| if (parentWindow) { | ||
| const sdkVersion = '1.13.0'; | ||
| parentWindow.postMessage({ | ||
| message: 'SDKVersion', | ||
| accountId, | ||
| originUrl: window.location.href, | ||
| sdkVersion | ||
| }, '*'); | ||
| break; | ||
|
|
||
| case WVE_QUERY_PARAMS.SDK_CHECK: | ||
| if (parentWindow) { | ||
| logger.debug('SDK version check'); | ||
| const sdkVersion = '1.13.1'; | ||
| parentWindow.postMessage({ | ||
| message: 'SDKVersion', | ||
| accountId, | ||
| originUrl: window.location.href, | ||
| sdkVersion | ||
| }, '*'); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| default: | ||
| logger.debug("unknown query param ".concat(ctType)); | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
| }; // TODO: Add a guarding mechanism to skip postMessages from non trusted sources |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard postMessages from untrusted sources.
The handleActionMode function currently sends and listens to postMessage events without verifying the sender’s domain. Although there's a TODO comment hinting at a fix, this is potentially a security risk.
Want help adding domain checks or a robust validation mechanism to mitigate any XSS or phishing vectors?
Changes
WEB-3502 - Added ctActionMode query paramater to handle all url paramters
Changes to Public Facing API if any
NA
How Has This Been Tested?
Dev + QA Tested
Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation / Chores