Skip to content

Conversation

@bredmond5
Copy link
Contributor

@bredmond5 bredmond5 commented Oct 6, 2025

Pull Request Template

Description

A customer has said that they are getting a bunch of NPEs on this line and that adding this check will alleviate some problems for them

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit test
  • Integration test

JS Budget Check

Please mention the size in kb before abd after this PR

Files Before After
dist/build.js.
dist/build.min.js

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Mentions:

List the person or team responsible for reviewing proposed changes.

cc @BranchMetrics/saas-sdk-devs for visibility.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2025

Test Results

  1 files  ±0    1 suites  ±0   54s ⏱️ ±0s
159 tests ±0  152 ✅ ±0  7 💤 ±0  0 ❌ ±0 
162 runs  ±0  155 ✅ ±0  7 💤 ±0  0 ❌ ±0 

Results for commit f9647fb. ± Comparison against base commit 34b3814.

@bredmond5 bredmond5 requested a review from mack-branch October 6, 2025 21:46
@matter-code-review
Copy link

Code Quality bug fix Reliability

Pull Request Template

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

This Pull Request introduces crucial null checks for parentNode and parentElement properties before attempting to remove child elements from the DOM. Specifically, banner_utils.removeElement now verifies element.parentNode and branch_view.displayJourney checks branchCSS.parentElement to prevent runtime errors. A minor formatting change also removed a blank line in session.set.

🔍 Impact of the Change

These changes significantly enhance the robustness and stability of the application by preventing potential TypeError exceptions (e.g., "Cannot read properties of null (reading 'removeChild')") that could occur if a parent node is unexpectedly null or undefined. This improves the overall reliability of DOM manipulation operations.

📁 Total Files Changed

  • src/2_session.js: Removed a blank line.
  • src/3_banner_utils.js: Added a null check for element.parentNode before removeChild.
  • src/branch_view.js: Added a null check for branchCSS.parentElement before removeChild.

🧪 Test Added

N/A

🔒Security Vulnerabilities

No new security vulnerabilities were introduced. The added null checks improve the application's defensive programming posture, indirectly contributing to stability and resilience against unexpected states.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

N/A

JS Budget Check

Files Before After
dist/build.js. N/A N/A
dist/build.min.js N/A N/A

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Mentions:

N/A

Tip

Quality Recommendations

  1. Consider implementing a consistent utility function for safe DOM element removal to centralize this defensive pattern across the codebase.

  2. Add unit tests specifically for banner_utils.removeElement and the relevant part of branch_view.displayJourney to cover scenarios where parentNode or parentElement might be null.

Tanka Poem ♫

Null checks now stand,
DOM's fragile dance made strong,
No more crashes seen.
Code's robust, a silent win,
Stability, a joyful hum. ✨

Sequence Diagram

sequenceDiagram
    participant App as Application Code
    participant BU as banner_utils
    participant BV as branch_view
    participant DOM as Document Object Model

    App->>BU: removeElement(element)
    Note over BU: Checks if element and element.parentNode exist
    alt element && element.parentNode
        BU->>DOM: element.parentNode.removeChild(element)
    else element or !element.parentNode
        Note over BU: No action taken
    end

    App->>BV: displayJourney(html, requestData, templateId, branchViewData)
    Note over BV: Retrieves branch-iframe-css element
    BV->>DOM: document.getElementById('branch-iframe-css')
    DOM-->>BV: branchCSS (element or null)
    Note over BV: Checks if branchCSS and branchCSS.parentElement exist
    alt branchCSS && branchCSS.parentElement
        BV->>DOM: branchCSS.parentElement.removeChild(branchCSS)
    else branchCSS or !branchCSS.parentElement
        Note over BV: No action taken
    end
Loading

@matter-code-review
Copy link

Important

PR Review Skipped

PR review skipped as per the configuration setting. Run a manually review by commenting /matter review

💡Tips to use MatterAI

Command List

  • /matter summary: Generate AI Summary for the PR
  • /matter review: Generate AI Reviews for the latest commit in the PR
  • /matter review-full: Generate AI Reviews for the complete PR
  • /matter release-notes: Generate AI release-notes for the PR
  • /matter : Chat with your PR with MatterAI Agent
  • /matter remember : Generate AI memories for the PR
  • /matter explain: Get an explanation of the PR
  • /matter help: Show the list of available commands and documentation
  • Need help? Join our Discord server: https://discord.gg/fJU5DvanU3

@bredmond5 bredmond5 merged commit 8a10a5c into main Oct 6, 2025
7 checks passed
@bredmond5 bredmond5 deleted the inteng-23549 branch October 6, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants