fix: ThemeSettingsStore singleton via React Context

[paul-phan]

Summary

• ThemeSettingsStore was creating a new instance on every useThemeSettingsStore() call, causing duplicate stores and state divergence
• Split into useCreateThemeSettingsStore (provider-side, creates singleton via useRef) and useThemeSettingsStore (consumer-side, reads from React Context)
• Theme data synced on navigation via useEffect instead of during render (fixes "Cannot update component while rendering" React warning)
• Fallback path preserved for older themes still using withWeaverse(App) instead of withWeaverse(Layout)

Test plan

• 15/15 unit tests pass (6 new tests for sync behavior and constructor edge cases)
• Hydrogen package builds successfully
• TypeScript typechecks pass
• Verify pilot template renders without "Cannot update component" warning
• Verify theme settings update correctly on client-side navigation

[claude]

Claude finished @paul-phan's task in 3m 38s —— View job

---

Code Review Summary

Overall: This PR is ready to merge. The fix properly addresses the singleton pattern bug using correct React patterns (Context + useRef + useEffect), maintains backward compatibility, and includes good test coverage. The code follows AGENTS.md conventions and has no blocking issues.

Issues

🔴 None found — No blocking issues identified.

Suggestions

🟡 packages/hydrogen/src/utils/use-theme-settings-store.ts:137 — The non-null assertion fallbackRef.current! is technically correct (the condition above guarantees at least one of fromContext or fallbackRef.current exists), but it relies on implicit logic that could be more explicit. Consider:

// Current code - correct but implicit
const store = fromContext ?? fallbackRef.current!

// More explicit alternative
const store = fromContext ?? fallbackRef.current ?? (() => {
  const s = new ThemeSettingsStore(data?.weaverseTheme)
  fallbackRef.current = s
  return s
})()

This makes it crystal clear that store can never be null, even if TypeScript can't infer it from the previous condition.

💬 packages/hydrogen/src/utils/use-theme-settings-store.ts:110 — The removed comment // Notify all listeners on line 47 was actually useful documentation for the emit() method. While it's obvious what the code does, having the comment explaining why (to notify all subscribed listeners of state changes) would help future maintainers understand the pub/sub pattern more quickly.

What's Good

• Correct React patterns: Proper use of useRef for singleton, useContext for sharing state, and useEffect for side effects (fixes the "Cannot update component while rendering" warning)
• Backward compatibility: Fallback path in useThemeSettingsStore() ensures older themes using withWeaverse(App) still work
• Excellent test coverage: 6 new tests covering sync behavior, constructor edge cases, and the shallow copy guarantee
• TypeScript best practices: Proper type annotations, no any abuses, explicit return types on public functions
• Follows monorepo conventions: Uses let (not const), single quotes, proper import patterns from @weaverse/react
• Clean API design: Separation of concerns between provider-side (useCreateThemeSettingsStore) and consumer-side (useThemeSettingsStore) is clear and intentional

---

Agent Fix Prompts

Copy any prompt below and send it to your coding agent to fix the issue.

🟡 Make null safety explicit

In packages/hydrogen/src/utils/use-theme-settings-store.ts at line 137: The non-null assertion on fallbackRef.current is correct but implicit. Make the null handling more explicit by providing a clear fallback that creates the store if needed, making it obvious to TypeScript and future readers that store can never be null. Use an immediately invoked function expression or conditional to make the logic crystal-clear.

💬 Restore useful comment

In packages/hydrogen/src/utils/use-theme-settings-store.ts at line 47: Add back the comment "// Notify all listeners" above the this.emit() call in updateThemeSettings(). This comment documents the intent of the pub/sub pattern and helps future maintainers understand why we're calling emit() after updating settings.

---