Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(platform): Added optional chaining due to strict null check #413

Conversation

unamdev0
Copy link
Contributor

@unamdev0 unamdev0 commented Sep 7, 2024

User description

Description

Added optional chaining in platform code due to strict null check implementation

Fixes #412


PR Type

enhancement, bug_fix


Description

  • Added optional chaining across multiple files to handle potential null or undefined values safely.
  • Implemented checks for localStorage to prevent errors in environments where it is not available.
  • Updated package.json to include @keyshade/api-client in dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
page.tsx
Add localStorage check and optional chaining                         

apps/platform/src/app/(main)/page.tsx

  • Added check for localStorage before accessing it.
  • Used optional chaining for safer JSON parsing.
  • +6/-3     
    page.tsx
    Use optional chaining for secrets mapping                               

    apps/platform/src/app/(main)/project/[project]/@secret/page.tsx

    • Added optional chaining to allSecrets mapping.
    +1/-1     
    layout.tsx
    Use optional chaining for project name access                       

    apps/platform/src/app/(main)/project/[project]/layout.tsx

    • Added optional chaining to access currentProject.name.
    +1/-1     
    page.tsx
    Use optional chaining for input focus                                       

    apps/platform/src/app/auth/invite/page.tsx

    • Added optional chaining to inputRef.current focus calls.
    +3/-3     
    index.tsx
    Use optional chaining for user name access                             

    apps/platform/src/components/shared/navbar/index.tsx

    • Added optional chaining to safely access data.name.
    +1/-1     
    index.tsx
    Use Boolean for conditional rendering                                       

    apps/platform/src/components/teams/teamTable/index.tsx

    • Used Boolean for conditional rendering logic.
    +1/-1     
    line-tab.tsx
    Use optional chaining for search comparison                           

    apps/platform/src/components/ui/line-tab.tsx

    • Added optional chaining for search comparison.
    +1/-1     
    env.ts
    Use optional chaining for error handling                                 

    apps/platform/src/env.ts

    • Added optional chaining to handle potential undefined errors.
    +1/-1     
    workspace-storage.ts
    Add localStorage checks for workspace storage                       

    apps/platform/src/lib/workspace-storage.ts

    • Added checks for localStorage before accessing it.
    +8/-3     
    middleware.ts
    Use optional chaining for cookie value access                       

    apps/platform/src/middleware.ts

    • Added optional chaining for cookie value access.
    +1/-1     
    Dependencies
    1 files
    package.json
    Update dependencies in package.json                                           

    package.json

    • Added @keyshade/api-client to dependencies.
    +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The optional chaining operator is used on allSecrets, but it's not clear where this variable is defined or if it can be null/undefined.

    Unnecessary Boolean Conversion
    The Boolean() function is unnecessarily used on an already boolean expression.

    Inconsistent Null Checks
    The function checks for localStorage availability inconsistently. Some checks use typeof localStorage !== 'undefined', while others use typeof localStorage!=="undefined".

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Sep 7, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Code organization
    ✅ Consolidate localStorage availability checks to reduce code duplication
    Suggestion Impact:The commit consolidated the localStorage availability check by removing the duplicate check within the nested if statement, aligning with the suggestion to reduce code duplication.

    code diff:

       if (typeof localStorage !== 'undefined') {
         localStorage.setItem('defaultWorkspace', JSON.stringify(defaultWorkspace))
    -  }
    -  if (getCurrentWorkspace() === null) {
    -    if (typeof localStorage !== 'undefined') {
    +
    +    if (getCurrentWorkspace() === null) {
           localStorage.setItem('currentWorkspace', JSON.stringify(defaultWorkspace))
         }
       }

    Consider using a single check for typeof localStorage !== 'undefined' at the
    beginning of the function to improve readability and reduce code duplication.

    apps/platform/src/lib/workspace-storage.ts [6-12]

     if (typeof localStorage !== 'undefined') {
       localStorage.setItem('defaultWorkspace', JSON.stringify(defaultWorkspace))
    -}
    -if (getCurrentWorkspace() === null) {
    -  if (typeof localStorage !== 'undefined') {
    +  if (getCurrentWorkspace() === null) {
         localStorage.setItem('currentWorkspace', JSON.stringify(defaultWorkspace))
       }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion improves code readability and organization by consolidating the typeof localStorage !== 'undefined' check, reducing code duplication and making the function more concise.

    9
    Simplification
    Simplify boolean expression by removing unnecessary Boolean() conversion

    The Boolean() function is unnecessary here as the table.getIsSomePageRowsSelected()
    already returns a boolean. Consider simplifying the expression.

    apps/platform/src/components/teams/teamTable/index.tsx [92]

    -Boolean(table.getIsSomePageRowsSelected() && 'indeterminate')
    +table.getIsSomePageRowsSelected() && 'indeterminate'
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid as it simplifies the code by removing the unnecessary Boolean() conversion, which is redundant since table.getIsSomePageRowsSelected() already returns a boolean.

    8
    Enhancement
    Use nullish coalescing for more precise fallback handling

    Consider using nullish coalescing (??) instead of logical OR (||) for the fallback
    value. This will only use the fallback if data.name is null or undefined, not for
    other falsy values like an empty string.

    apps/platform/src/components/shared/navbar/index.tsx [41]

    -name: data.name?.split(' ')[0] ?? data.email.split('@')[0],
    +name: data.name?.split(' ')[0] ?? data.email?.split('@')[0] ?? '',
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies that using nullish coalescing (??) instead of logical OR (||) provides a more precise fallback handling, as it only considers null or undefined values, not other falsy values like an empty string.

    7

    @unamdev0
    Copy link
    Contributor Author

    unamdev0 commented Sep 7, 2024

    @rajdip-b please note that other than optional chaining, I had to add checks here and here, cause after building, code was throwing reference error to localStorage , this was also in fact due to strict null check.

    Screenshot 2024-09-06 at 9 25 07 PM

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Sep 7, 2024

    Yes, this was a known issue. Thanks for getting this solved!

    @rajdip-b rajdip-b changed the title refactor(platform): added optional chaining due to strict null check chore(platform): Added optional chaining due to strict null check Sep 7, 2024
    @rajdip-b rajdip-b merged commit 907e369 into keyshade-xyz:develop Sep 7, 2024
    6 checks passed
    @unamdev0 unamdev0 deleted the fix/412-test-failing-due-to-strict-check-implementation branch September 7, 2024 13:34
    rajdip-b pushed a commit that referenced this pull request Sep 16, 2024
    ## [2.5.0](v2.4.0...v2.5.0) (2024-09-16)
    
    ### 🚀 Features
    
    * **api-client:** Added workspace controller ([#427](#427)) ([2f4edec](2f4edec))
    * **api-client:** Added workspace role controller ([#430](#430)) ([b03ce8e](b03ce8e))
    * **api-client:** Synced with latest API ([27f4309](27f4309))
    * **api:** Add slug in entities ([#415](#415)) ([89e2fcc](89e2fcc))
    * **api:** Included default workspace details in getSelf function ([#414](#414)) ([e67bbd6](e67bbd6))
    * **platform:** Add loading skeleton in the [secure]s page ([#423](#423)) ([a97681e](a97681e))
    * **schema:** Added a schema package ([01ea232](01ea232))
    * **web:** Update about and careers page ([e167f53](e167f53))
    
    ### 🐛 Bug Fixes
    
    * **api:** Error messages fixed in api-key service ([#418](#418)) ([edfbce0](edfbce0))
    
    ### 📚 Documentation
    
    * Fixed minor typo in postman workspace link ([#411](#411)) ([ed23116](ed23116))
    * Updated Postman links ([444bfb1](444bfb1))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Suppressed version check test in [secure] ([4688e8c](4688e8c))
    * **api:** Update slug generation method ([#420](#420)) ([1f864df](1f864df))
    
    ### 🔨 Code Refactoring
    
    * **API:** Refactor workspace-membership into a separate module ([#421](#421)) ([574170f](574170f))
    * **platform:** added optional chaining due to strict null check ([#413](#413)) ([907e369](907e369))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.5.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Test failing due to strict null check implementation
    2 participants