usermaven / usermaven-js

Usermaven provides instant actionable analytics to grow your SaaS business.
MIT License
1 stars 2 forks source link

feat: setUserId added #126

Closed seeratawan01 closed 2 weeks ago

seeratawan01 commented 2 weeks ago

PR Type

enhancement, tests


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
client.ts
Add `setUserId` method and improve code formatting             

packages/javascript-sdk/src/core/client.ts
  • Added a new method setUserId to update user ID in persistence and user
    properties.
  • Adjusted spacing and formatting for consistency.
  • +29/-17 
    Tests
    cross-domain.html
    Add UI elements and logic for user ID update                         

    packages/javascript-sdk/examples/cross-domain.html
  • Added buttons for tracking events and updating user ID.
  • Implemented JavaScript to handle user ID updates via prompt.
  • +15/-0   

    ๐Ÿ’ก PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    github-actions[bot] commented 2 weeks ago

    PR Reviewer Guide ๐Ÿ”

    Here are some key observations to aid the review process:

    ๐Ÿ… Score: 85
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ No security concerns identified
    โšก Recommended focus areas for review

    Code Duplication
    The new code introduces some duplication in handling persistence updates, which could be refactored into a more modular approach. Error Handling
    The new method setUserId lacks error handling which might be necessary to deal with potential issues during persistence operations.
    Code feedback:
    relevant filepackages/javascript-sdk/src/core/client.ts
    suggestion       Consider using a single method to handle the persistence of user properties to avoid code duplication and ensure consistency. For example, you could create a method `updateUserProperties` that handles all updates related to user properties. [important]
    relevant linethis.persistence.set('userProps', userProps);

    relevant filepackages/javascript-sdk/src/core/client.ts
    suggestion       It's recommended to add error handling for the persistence operations in the `setUserId` method to ensure the application can gracefully handle potential issues such as storage limits being exceeded. [important]
    relevant linethis.persistence.save();

    github-actions[bot] commented 2 weeks ago

    PR Code Suggestions โœจ

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Reliability
    Implement error handling in the setUserId method to enhance reliability ___ **Add error handling for the setUserId method to manage cases where persistence
    operations fail, ensuring the application's stability.** [packages/javascript-sdk/src/core/client.ts [563-571]](https://github.com/usermaven/usermaven-js/pull/126/files#diff-cf1b46df259d6f5f7aa4358f3de9b0e4ca7051836cc3f55729ad1c56a5f7ac40R563-R571) ```diff -this.persistence.set('userId', userId); -... -this.persistence.save(); +try { + this.persistence.set('userId', userId); + ... + this.persistence.save(); +} catch (error) { + this.logger.error('Failed to set user ID:', error); +} ```
    Suggestion importance[1-10]: 9 Why: Adding error handling to the `setUserId` method is crucial for improving the application's reliability, as it ensures that any failures in persistence operations are logged and managed appropriately.
    9
    Possible bug
    Validate the userId to ensure it is not an empty string before using it ___ **Ensure that the userId is a non-empty string before setting it in the persistence
    and user properties to avoid potential issues with invalid user identifiers.** [packages/javascript-sdk/src/core/client.ts [563-568]](https://github.com/usermaven/usermaven-js/pull/126/files#diff-cf1b46df259d6f5f7aa4358f3de9b0e4ca7051836cc3f55729ad1c56a5f7ac40R563-R568) ```diff -this.persistence.set('userId', userId); -... -this.persistence.set('userProps', userProps); +if (userId.trim() !== '') { + this.persistence.set('userId', userId); + ... + this.persistence.set('userProps', userProps); +} ```
    Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential bug by ensuring that the `userId` is not an empty string before setting it in persistence, which can prevent invalid user identifiers from being stored and causing issues later.
    8
    Best practice
    Check if history.pushState is already overridden to prevent conflicts ___ **Consider checking if history.pushState is already overridden to avoid potential
    conflicts or errors in environments where multiple scripts might modify browser
    history.** [packages/javascript-sdk/src/core/client.ts [504-507]](https://github.com/usermaven/usermaven-js/pull/126/files#diff-cf1b46df259d6f5f7aa4358f3de9b0e4ca7051836cc3f55729ad1c56a5f7ac40R504-R507) ```diff -history.pushState = function () { - trackPageLeave(); - return originalPushState.apply(this, arguments as any); -}; +if (history.pushState !== originalPushState) { + console.warn('history.pushState was already overridden.'); +} else { + history.pushState = function () { + trackPageLeave(); + return originalPushState.apply(this, arguments as any); + }; +} ```
    Suggestion importance[1-10]: 7 Why: This suggestion is a best practice that helps avoid conflicts in environments where multiple scripts might modify browser history, ensuring that the `pushState` method is not overridden multiple times.
    7
    Enhancement
    Use a deep merge utility to handle nested properties correctly ___ **Use a more robust method for merging properties to handle potential nested
    properties correctly in user and global properties.** [packages/javascript-sdk/src/core/client.ts [542-546]](https://github.com/usermaven/usermaven-js/pull/126/files#diff-cf1b46df259d6f5f7aa4358f3de9b0e4ca7051836cc3f55729ad1c56a5f7ac40R542-R546) ```diff -props = {...props, ...properties}; +import merge from 'lodash/merge'; ... -globalProps = {...globalProps, ...properties}; +props = merge(props, properties); +... +globalProps = merge(globalProps, properties); ```
    Suggestion importance[1-10]: 6 Why: Using a deep merge utility like `lodash/merge` can enhance the robustness of property merging, especially if nested properties are involved, improving the maintainability and correctness of the code.
    6