fix: fixing display name issues #1908
Conversation
* feat: sanitize input data * feat: sanitize all user inputs * fix: exclude sensitive data auth token from bugsnag metadata * chore: address ai bot review comments * refactor: move utilities to the correct location and add tests * chore: revert unnecessary changes
* feat: add date datatype * chore: update allowed datatypes * chore: address review comment
📝 WalkthroughWalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## hotfix/nov07.2024 #1908 +/- ##
=====================================================
- Coverage 57.49% 57.47% -0.02%
=====================================================
Files 485 485
Lines 16493 16495 +2
Branches 3291 3304 +13
=====================================================
- Hits 9482 9481 -1
+ Misses 5792 5745 -47
- Partials 1219 1269 +50 ☔ View full report in Codecov by Sentry. |
|
![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
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts (1)
3-3: Add test coverage for the constants.The static analysis indicates that the constants are not covered by tests. Consider adding tests to verify the correct export and mapping of these constants.
Would you like me to help generate test cases for these constants? I can create a test suite that verifies:
- The correct value of DISPLAY_NAME
- The mapping in DISPLAY_NAME_TO_DIR_NAME_MAP
- All variations in CNameMapping
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts#L3
Added line #L3 was not covered by testspackages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts (1)
3-3: Add test coverage for the display name constant.The DISPLAY_NAME constant is not covered by tests. Consider adding integration tests to verify:
- Correct display name in destination UI
- Proper mapping in destination configurations
Would you like me to help generate test cases for this constant?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts#L3
Added line #L3 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js(1 hunks)packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts(1 hunks)packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts(1 hunks)packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts#L3
Added line #L3 was not covered by tests
🔇 Additional comments (5)
packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts (1)
3-3: Add test coverage for DISPLAY_NAME constant
The DISPLAY_NAME constant is crucial for destination loading but lacks test coverage. Please add tests to verify the mapping works correctly with the integration.
Would you like me to help create test cases for this constant?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts (1)
3-3: LGTM! Verify the display name change impact.
The standardization of DISPLAY_NAME to 'Convertflow' aligns with the PR's objective of fixing display name issues. The CNameMapping already handles this case variation appropriately.
Let's verify the usage of this constant across the codebase:
✅ Verification successful
Display name change is safe to proceed
The change from 'ConvertFlow' to 'Convertflow' is safe because:
- The
CNameMappingin constants.ts already handles all case variations including 'Convertflow' - The browser.js integration only uses
DISPLAY_NAMEfor logging purposes - The integration_cname.js and destinationNames.ts files import and use the constant correctly through proper exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct string references that might need updating
# and verify the usage of DISPLAY_NAME constant
# Search for any hardcoded references to 'ConvertFlow' that might need updating
rg -l 'ConvertFlow' --type ts --type js
# Check imports and usage of DISPLAY_NAME from this constants file
rg -A 2 "from ['|\"].*ConvertFlow/constants" --type ts --type js
Length of output: 2121
Script:
#!/bin/bash
# Let's check the actual content of the files that import DISPLAY_NAME
# to ensure they're using it correctly and won't be affected by the case change
# Check browser.js implementation
cat packages/analytics-js-integrations/src/integrations/ConvertFlow/browser.js
# Check integration_cname.js to see CNameMapping usage
cat packages/analytics-js-common/src/constants/integrations/integration_cname.js
# Check destinationNames.ts to see how DISPLAY_NAME is used
cat packages/analytics-js-common/src/constants/integrations/destinationNames.ts
# Also check the original constants file to see full context
cat packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts
Length of output: 18933
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ConvertFlow/constants.ts#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js (2)
3-3: LGTM! Display name update matches official branding.
The change from 'Active Campaign' to 'ActiveCampaign' aligns with the official product branding while maintaining backward compatibility through CNameMapping.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js#L3
Added line #L3 was not covered by tests
3-3:
Add test coverage for the DISPLAY_NAME constant.
The static analysis indicates that this line isn't covered by tests. Since display names are critical for integration identification, we should ensure proper test coverage.
Let's check existing test coverage:
Would you like me to help generate test cases to cover this constant?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/ActiveCampaign/constants.js#L3
Added line #L3 was not covered by tests
packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts (1)
Line range hint 5-14: LGTM! Comprehensive mapping coverage.
The mappings appropriately handle all variations of the name, maintaining backward compatibility while supporting the new display name.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts#L3
Added line #L3 was not covered by tests
| @@ -1,6 +1,6 @@ | |||
| const DIR_NAME = 'LiveChat'; | |||
| const NAME = 'LIVECHAT'; | |||
| const DISPLAY_NAME = 'LiveChat'; | |||
| const DISPLAY_NAME = 'livechat'; | |||
There was a problem hiding this comment.
Maintain proper casing for company name "LiveChat"
The change to lowercase 'livechat' appears incorrect as LiveChat is a company name and should maintain proper casing. This is consistent with how other integration display names are handled (e.g., ActiveCampaign, Lemnisk Marketing Automation).
Apply this diff to fix the casing:
-const DISPLAY_NAME = 'livechat';
+const DISPLAY_NAME = 'LiveChat';📝 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 DISPLAY_NAME = 'livechat'; | |
| const DISPLAY_NAME = 'LiveChat'; |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/LiveChat/constants.ts#L3
Added line #L3 was not covered by tests
| @@ -1,6 +1,6 @@ | |||
| const DIR_NAME = 'Lemnisk'; | |||
| const NAME = 'LEMNISK'; | |||
| const DISPLAY_NAME = 'Lemnisk'; | |||
| const DISPLAY_NAME = 'Lemnisk Marketing Automation'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Update required: Inconsistent display name found in multiple files
The display name change from 'Lemnisk' to 'Lemnisk Marketing Automation' needs to be synchronized across these files:
packages/analytics-js-common/src/constants/integrations/config_to_integration_names.jspackages/analytics-js-common/src/constants/integrations/client_server_name.jsexamples/chrome-extension/content-script-v1.1/foreground.jsexamples/chrome-extension/content-script-v3/foreground.js
🔗 Analysis chain
LGTM! Verify impact on destination configurations.
The more descriptive display name aligns with the standardization effort across integration files.
Let's verify if this display name is consistently used across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old display name 'Lemnisk'
# that might need updating
rg -l "['\"](Lemnisk)['\"]" --type ts --type js
Length of output: 413
Script:
#!/bin/bash
# Let's check the actual content and context of these occurrences
echo "=== Checking config_to_integration_names.js ==="
rg -A 2 -B 2 "Lemnisk" packages/analytics-js-common/src/constants/integrations/config_to_integration_names.js
echo -e "\n=== Checking client_server_name.js ==="
rg -A 2 -B 2 "Lemnisk" packages/analytics-js-common/src/constants/integrations/client_server_name.js
echo -e "\n=== Checking chrome extension files ==="
rg -A 2 -B 2 "Lemnisk" examples/chrome-extension/content-script-v1.1/foreground.js
rg -A 2 -B 2 "Lemnisk" examples/chrome-extension/content-script-v3/foreground.js
Length of output: 125778
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: packages/analytics-js-common/src/constants/integrations/Lemnisk/constants.ts#L3
Added line #L3 was not covered by tests
size-limit report 📦
|
|
we are handling it in #1909 |
PR Description
While verifying the device mode destinations loading across the entire repository we found out that the below destinations were not loading due to display name mismatch:
Linear task (optional)
https://linear.app/rudderstack/issue/INT-2811/test-web-device-mode-destinations-for-proper-loading
Resolves INT-2811
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit