Skip to content

fix: remove ThemeSettingsStore listener cap and optimize Pilot badge components (#431)#439

Merged
paul-phan merged 6 commits into
mainfrom
fix/431-theme-settings-store-max-listeners
Mar 6, 2026
Merged

fix: remove ThemeSettingsStore listener cap and optimize Pilot badge components (#431)#439
paul-phan merged 6 commits into
mainfrom
fix/431-theme-settings-store-max-listeners

Conversation

@paul-phan
Copy link
Copy Markdown
Member

Summary

Fixes #431 - removes the hard MAX_LISTENERS=100 cap that silently dropped subscriptions when exceeded, and optimizes Pilot badge components to reduce store subscriptions.

Changes

SDK (packages/hydrogen)

  • Removed hard "Maximum listeners exceeded" cap that silently dropped new subscriptions
  • Added advisory-only dev warning at >500 listeners (fires once, never blocks)
  • New tests: 9 comprehensive ThemeSettingsStore tests

Pilot Template (templates/pilot submodule)

  • Refactored badge components to receive settings as props (pure components)
  • ProductBadges now calls useThemeSettings() once and passes to children
  • Reduced subscriptions per product card: 7 → 2
  • Product grid (10 items): ~70 listeners → ~20 listeners

Impact

  • No more silent subscription drops - all listeners work correctly
  • Prevents "Maximum listeners exceeded" warning on pages with product grids
  • Cleaner architecture: parent fetches, children receive via props

Testing

  • ThemeSettingsStore tests: 9/9 pass
  • LSP: 0 errors in all modified files
  • No other files import individual badge components

Verification

bun test v1.3.9 (cf6cdbbb)

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 5, 2026

⚠️ No Changeset found

Latest commit: a527cd9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

Claude finished @paul-phan's task in 1m 6s —— View job


Code Review Summary

Overall: ✅ LGTM - Ready to merge. This PR correctly fixes issue #431 by removing the hard listener cap that silently dropped subscriptions, replacing it with an advisory-only warning. The implementation is clean, well-tested, and properly versioned.


Analysis

Core Fix (packages/hydrogen/src/utils/use-theme-settings-store.ts)

Before: Hard cap at 100 listeners - subscriptions were silently dropped when exceeded

private readonly MAX_LISTENERS = 100
if (this.listeners.size >= this.MAX_LISTENERS) {
  console.warn(...)  // Warning only, then returns no-op
  return () => {}    // ❌ SUBSCRIPTION DROPPED
}

After: Advisory warning at 500+ listeners - all subscriptions work

private hasWarnedListenerCount = false
this.listeners.add(callback)  // ✅ ALWAYS added
if (!this.hasWarnedListenerCount && this.listeners.size > 500) {
  this.hasWarnedListenerCount = true
  console.warn(...)  // Advisory only, never blocks
}

Test Coverage (packages/hydrogen/__tests__/theme-settings-store.test.ts)

  • ✅ 9 comprehensive tests following AAA pattern
  • ✅ Tests the 200 listener scenario (proves cap removal works)
  • ✅ Tests warning threshold behavior
  • ✅ Tests error handling and edge cases
  • ✅ Uses let declarations (follows biome config)
  • ✅ Proper beforeEach/afterEach cleanup with spy mocks

Version Management

  • ✅ All three packages synchronized: 5.9.25.9.3
    • @weaverse/core: 5.9.3
    • @weaverse/react: 5.9.3 (depends on @weaverse/core 5.9.3)
    • @weaverse/hydrogen: 5.9.3 (depends on @weaverse/react 5.9.3)
  • ✅ Dependency chain properly maintained

Pilot Template

  • Updated submodule commit for badge component optimization
  • Reduces subscriptions: 7 → 2 per product card
  • (Submodule changes are not visible in PR diff but referenced in summary)

What's Good

  1. Defensive programming: emit() creates a copy of listeners before iteration - prevents issues if listeners are modified during iteration
  2. One-time warning: hasWarnedListenerCount flag prevents console spam
  3. Helpful warning message: Suggests "Consider using fewer useThemeSettings() calls" to guide developers toward the fix (props drilling)
  4. Comprehensive tests: The 200-listener test directly validates the bug fix

Minor Suggestions

💬 packages/hydrogen/src/utils/use-theme-settings-store.ts:59 — Consider adding a comment explaining why 500 was chosen as the warning threshold (e.g., "Arbitrary threshold to detect potential overuse while allowing legitimate product grid use cases")

💬 No action needed, but the test naming should_notify_all_200_listeners_on_update could be should_notify_all_200_listeners_when_settings_updated for perfect consistency with the when_[StateUnderTest] pattern mentioned in AGENTS.md


Conclusion

No blocking issues. The fix is minimal, targeted, and correctly addresses the silent subscription drop problem. The comprehensive test suite validates the fix and prevents regression.


…ns across core, react, and hydrogen packages.
@paul-phan paul-phan merged commit 7a432c8 into main Mar 6, 2026
8 checks passed
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.

Fix: ThemeSettingsStore Maximum listeners (100) exceeded - Possible memory leak

1 participant