Skip to content

Conversation

@PGrayCS
Copy link

@PGrayCS PGrayCS commented Sep 30, 2025

Description

Adds null check for activeIndicator to match the pattern used for newActiveIndicator on line 282.

Motivation & Context

While testing carousel behavior with modified DOM, I noticed SelectorEngine.findOne() can return null if there's no active indicator, which would cause an error on classList.remove(). This adds the same safety check that's already in place for newActiveIndicator.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Related issues

N/A

Prevents potential null reference error in _setActiveIndicatorElement when
no active indicator exists in the carousel.
@PGrayCS PGrayCS requested a review from a team as a code owner September 30, 2025 21:08
@XhmikosR
Copy link
Member

XhmikosR commented Oct 1, 2025

This smells like AI.. Why would this be null in the first place? Also, most if not all JS changes need an accompanied test case.

@PGrayCS
Copy link
Author

PGrayCS commented Oct 3, 2025

Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.

Happy to add a test case if you think this scenario is worth covering, or close this if you'd rather keep it as-is since it might be an edge case.

@julien-deramond
Copy link
Member

Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.

Could you please provide a reproducible use case so we can test it manually, and if possible, include the corresponding unit test in this PR?

Covers scenario where carousel has indicators but none are marked as
active initially. Ensures sliding works correctly and activates the
new indicator without errors.
@PGrayCS
Copy link
Author

PGrayCS commented Oct 4, 2025

Sure thing. This happens when the carousel is initialized programmatically and the indicators element exists but has no active indicator yet. In that case, SelectorEngine.findOne(SELECTOR_ACTIVE, this._indicatorsElement) returns null, and then the code tries to call .classList.remove() on null which throws a TypeError.

I ran into this when I was building indicators dynamically and hadn't synced up the active states properly before the first slide transition.

I've added a test case in the latest commit that covers this scenario - creates a carousel with indicators but none marked as active, then calls next(). Verifies it slides correctly and activates the new indicator without erroring.

The null check matches what's already there for newActiveIndicator on line 282, so it keeps the code consistent.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants