fix: improved theme logic

[ignaciosantise]

Summary

Improved theme and setTheme logic

---

Note

Refactors theme management to decouple system theme from user-defined default and compute themeMode from those sources.

• Introduces systemThemeMode and defaultThemeMode in ThemeController with a computed themeMode (priority: default > system > light), removes Appearance dependency, and adds setSystemThemeMode/setDefaultThemeMode
• Updates useAppKitTheme to expose non-optional themeMode and route setThemeMode to ThemeController.setDefaultThemeMode
• w3m-modal now wires system theme via ThemeController.setSystemThemeMode and passes resolved themeMode/themeVariables to ThemeProvider
• ThemeContext now requires themeMode and defaults to light when absent
• Test suites updated to reflect new API and precedence rules

^{Written by Cursor Bugbot for commit 5714a28. This will update automatically on new commits. Configure here.}

[changeset-bot]

🦋 Changeset detected

Latest commit: 5714a28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@reown/appkit-react-native	Patch
@reown/appkit-bitcoin-react-native	Patch
@reown/appkit-coinbase-react-native	Patch
@reown/appkit-common-react-native	Patch
@reown/appkit-core-react-native	Patch
@reown/appkit-ethers-react-native	Patch
@reown/appkit-solana-react-native	Patch
@reown/appkit-ui-react-native	Patch
@reown/appkit-wagmi-react-native	Patch

Not sure what this means? Click here to learn what changesets are.

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

[claude]

Claude finished @ignaciosantise's task —— View job

---

PR Review: Theme Logic Improvements

Summary

This PR improves the theme management architecture by introducing a derived state pattern using derive-valtio, separating system theme from user-defined theme, and simplifying the theme resolution logic.

---

Issues Found

1. Type Safety Issue in setSystemThemeMode - packages/core/src/controllers/ThemeController.ts:42

The setSystemThemeMode method has a subtle type safety issue:

setSystemThemeMode(systemThemeMode?: ThemeControllerState['systemThemeMode']) {
  baseState.systemThemeMode = systemThemeMode ?? 'light';
}

Problem: When systemThemeMode is explicitly null (not undefined), the nullish coalescing operator (??) won't trigger, and baseState.systemThemeMode will be set to null. However, the derived state's fallback logic expects systemThemeMode to be either ThemeMode or nullish:

return snap.defaultThemeMode ?? snap.systemThemeMode ?? 'light';

Impact: When systemThemeMode is null, this would incorrectly return null instead of falling back to 'light'.

Recommendation: Either:

• Change the type to disallow null: systemThemeMode?: ThemeMode (preferred)
• Or explicitly handle null in the setter: baseState.systemThemeMode = systemThemeMode || 'light'

---

2. Missing Mock Update in Tests - packages/appkit/src/__tests__/hooks/useAppKitTheme.test.tsx:28

The test mock doesn't include the setDefaultThemeMode method that's now being used:

jest.mock('@reown/appkit-core-react-native', () => ({
  ThemeController: {
    state: {
      themeMode: 'light',
      themeVariables: {}
    },
    setThemeMode: jest.fn(),  // ❌ Old method (unused)
    setThemeVariables: jest.fn()
  }
}));

Problem: The mock includes setThemeMode but should include setDefaultThemeMode to match the actual implementation.

Recommendation: Update the mock to:

jest.mock('@reown/appkit-core-react-native', () => ({
  ThemeController: {
    state: {
      themeMode: 'light',
      themeVariables: {}
    },
    setDefaultThemeMode: jest.fn(),  // ✅ Correct method
    setThemeVariables: jest.fn()
  }
}));

---

3. Documentation Comment Accuracy - packages/appkit/src/hooks/useAppKitTheme.ts:11

The JSDoc comment states:

/** The current theme mode ('dark' or 'light'), or undefined if using system default */
themeMode?: ThemeMode;

Problem: With the new derived state implementation, themeMode is never undefined. The derived state always returns a fallback value:

return snap.defaultThemeMode ?? snap.systemThemeMode ?? 'light';

Recommendation: Update the JSDoc to reflect reality:

/** The current theme mode ('dark' or 'light'). Defaults to 'light' if no theme is set. */
themeMode: ThemeMode;  // Remove the optional modifier

---

4. React Native Dependency Removal - packages/core/src/controllers/ThemeController.ts

Positive observation: The removal of the Appearance import from react-native in the core controller is good architectural practice. This moves the platform-specific logic to the UI layer (w3m-modal/index.tsx:38) where it belongs, making the core controller more portable and testable.

However, the w3m-modal/index.tsx:22 still imports useColorScheme:

const theme = useColorScheme();

Potential Issue: useColorScheme() can return null on initial render or when color scheme is unavailable. The current code passes this directly to setSystemThemeMode(theme), which would set it to null, triggering Issue #1 above.

Recommendation: Add null handling:

useEffect(() => {
  ThemeController.setSystemThemeMode(theme ?? undefined);
}, [theme]);

---

5. Missing Type Safety in Hook Return - packages/appkit/src/hooks/useAppKitTheme.ts:10-19

The return type interface declares themeMode as optional, but the derived state guarantees it's always defined:

export interface UseAppKitThemeReturn {
  themeMode?: ThemeMode;  // ❌ Shouldn't be optional
  // ...
}

Recommendation: Remove the optional modifier for consistency with the actual behavior:

export interface UseAppKitThemeReturn {
  themeMode: ThemeMode;  // ✅ Always defined
  // ...
}

---

Architecture & Best Practices

✅ Good Patterns

1. Separation of Concerns: The new architecture cleanly separates system theme (systemThemeMode) from user preference (defaultThemeMode)
2. Derived State Pattern: Using derive-valtio for computed theme mode is an excellent use of Valtio's reactivity system
3. Immutability: Properly spreading objects when merging theme variables
4. Function Stability: Using useMemo with .bind() for stable function references in the hook is correct
5. Test Coverage: Comprehensive test updates showing good testing discipline

📝 Suggestions

1. The naming could be clearer: consider userThemeMode instead of defaultThemeMode to better indicate it's user-set vs. default fallback
2. Consider adding JSDoc comments to the controller methods to document the theme resolution priority: user theme → system theme → 'light'

---

Security & Performance

• ✅ No security concerns identified
• ✅ Performance is good - derived state is efficient and only recomputes when dependencies change
• ✅ No unnecessary re-renders introduced

---

Missing Changeset

Note: This PR needs a changeset. Use the changeset-bot's provided link or run npx changeset locally to add one for @reown/appkit-core-react-native and @reown/appkit-react-native with semver type patch.

---

[Copilot]

Pull Request Overview

This PR refactors the theme management logic to introduce a clearer separation between system theme and user-defined default theme using valtio's derived state pattern.

Key Changes:

• Introduced derive-valtio to create a computed themeMode that prioritizes defaultThemeMode over systemThemeMode
• Renamed setThemeMode to setDefaultThemeMode for the user-facing API and added setSystemThemeMode for tracking system theme changes
• Updated the modal to always track system theme changes via setSystemThemeMode, while the hook API uses setDefaultThemeMode to respect user preferences

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
packages/core/src/controllers/ThemeController.ts	Refactored to use derived state pattern with separate system and default theme modes
packages/appkit/src/modal/w3m-modal/index.tsx	Updated to use setSystemThemeMode for tracking system theme changes
packages/appkit/src/hooks/useAppKitTheme.ts	Changed setThemeMode to call setDefaultThemeMode internally
packages/appkit/src/tests/hooks/useAppKitTheme.test.tsx	Updated test expectations to reflect new default behavior and API changes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Type inconsistency: systemThemeMode and defaultThemeMode are typed as ThemeMode | null but are initialized with undefined. This creates a mismatch between the type definition and the actual runtime values. Consider either:

1. Changing the type to ThemeMode | null | undefined or ThemeMode | undefined
2. Initializing with null instead of undefined on lines 14-15

This inconsistency could lead to type-safety issues when accessing these properties.

[Copilot]

The setSystemThemeMode function always defaults to 'light' when the parameter is undefined or null. However, when the system theme changes to null (which useColorScheme() can return), this will force it to 'light' rather than preserving the null value. This could prevent proper fallback to the default theme mode. Consider allowing null to be set when explicitly passed, or handle the React Native's useColorScheme() null return value more gracefully.

Suggested change

	baseState.systemThemeMode = systemThemeMode ?? 'light';
	baseState.systemThemeMode = systemThemeMode === undefined ? 'light' : systemThemeMode;

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ignaciosantise]

@claude review

[claude]

Claude finished @ignaciosantise's task —— View job

---

🔄 Incremental PR Review

Summary: 3 of 5 previously identified issues have been resolved. 1 minor type safety issue remains. 2 new minor issues found related to API clarity and documentation.

---

Validation of Previous Issues

Issue #1: Type Safety Issue in setSystemThemeMode (ThemeController.ts:42)

Status: ✅ RESOLVED

Previous concern was that when systemThemeMode is explicitly null, the nullish coalescing operator wouldn't trigger. However, examining the current implementation:

// Line 7-8
systemThemeMode?: ThemeMode;
defaultThemeMode?: ThemeMode;

The type signature uses ThemeMode | undefined (via optional ?), which does not allow null. The implementation at line 42:

baseState.systemThemeMode = systemThemeMode ?? 'light';

This is now type-safe because null cannot be passed to this function according to the TypeScript type system.

---

Issue #2: Missing Mock Update in Tests (useAppKitTheme.test.tsx:28)

Status: ✅ RESOLVED

The mock has been correctly updated at lines 27-28:

setDefaultThemeMode: jest.fn(),
setThemeVariables: jest.fn()

The old setThemeMode method is no longer in the mock, replaced with the correct setDefaultThemeMode.

---

Issue #3: Documentation Comment Accuracy (useAppKitTheme.ts:11-12)

Status: ⚠️ STILL PRESENT

The JSDoc comment and interface at lines 11-12 state:

/** The current theme mode ('dark' or 'light') */
themeMode?: ThemeMode;

The property is still marked as optional (?), but the derived state always returns a value (ThemeController.ts:25):

return snap.defaultThemeMode ?? snap.systemThemeMode ?? 'light';

Impact: Minor - consumers may perform unnecessary null checks.

Recommendation: Remove the optional modifier in useAppKitTheme.ts:12:

themeMode: ThemeMode;

---

Issue #4: React Native Dependency & Null Handling (w3m-modal/index.tsx:38)

Status: ✅ RESOLVED

The implementation at lines 37-39 correctly handles null:

useEffect(() => {
  ThemeController.setSystemThemeMode(theme ?? undefined);
}, [theme]);

This converts null from useColorScheme() to undefined before passing to setSystemThemeMode, which is the correct approach.

---

Issue #5: Missing Type Safety in Hook Return (useAppKitTheme.ts:10-19)

Status: ⚠️ STILL PRESENT

Same as Issue #3 - the interface declares themeMode as optional at line 12 but it's always defined in practice.

---

Copilot Review Comments Validation

Copilot Comment #1: Type inconsistency in initialization

Status: ✅ NO ISSUE

Copilot noted that systemThemeMode and defaultThemeMode are typed as ThemeMode | null but initialized with undefined. However, examining the current code (ThemeController.ts:7-8):

systemThemeMode?: ThemeMode;
defaultThemeMode?: ThemeMode;

The types are actually ThemeMode | undefined (via optional ?), not ThemeMode | null. The initialization at lines 14-15 with undefined is correct and consistent with the type definition.

This appears to be a misreading by Copilot.

---

Copilot Comment #2: setSystemThemeMode null handling

Status: ✅ RESOLVED

Already addressed by Issue #4 validation above - the w3m-modal correctly converts null to undefined.

---

New Issues Found

New Issue #1: Potentially Confusing setSystemThemeMode Default Behavior

Location: packages/core/src/controllers/ThemeController.ts:41-43
Severity: Low

The setSystemThemeMode method defaults to 'light' when called without arguments:

setSystemThemeMode(systemThemeMode?: ThemeControllerState['systemThemeMode']) {
  baseState.systemThemeMode = systemThemeMode ?? 'light';
}

However, this is inconsistent with how setDefaultThemeMode works (which allows undefined). Looking at the test at ThemeController.test.ts:63-67:

it('should default to light when called without arguments', () => {
  ThemeController.setSystemThemeMode();
  const state = ThemeController.state;
  expect(state.systemThemeMode).toBe('light');
});

This behavior seems intentional based on the test. However, it means you cannot clear the system theme mode once it's set, which could be problematic if you want to remove system theme tracking.

Recommendation: Consider whether setSystemThemeMode(undefined) should clear the system theme mode rather than setting it to 'light'. If the current behavior is intentional, add a JSDoc comment explaining why.

---

New Issue #2: Missing JSDoc on Controller Methods

Location: packages/core/src/controllers/ThemeController.ts:37-57
Severity: Low

The controller methods lack JSDoc comments explaining their purpose and behavior:

• subscribe - no documentation
• setSystemThemeMode - no documentation on why it defaults to 'light'
• setDefaultThemeMode - no documentation
• setThemeVariables - no documentation

Recommendation: Add JSDoc comments to clarify:

1. The theme resolution priority (default > system > 'light')
2. Why setSystemThemeMode defaults to 'light' when called without args
3. The difference between "system theme" and "default theme"

This would improve developer experience, especially since the naming (defaultThemeMode vs user-facing setThemeMode) could be clearer.

---

Overall Assessment

Previously Identified Issues:

• ✅ 3 issues resolved since November review
• ⚠️ 1 issue remains (optional themeMode type should be required)

New Issues:

• 2 minor issues found (confusing default behavior, missing JSDoc)

Code Quality: ✅ Good

• Proper use of derived state pattern
• Comprehensive test coverage
• Clean separation of concerns

Security: ✅ No concerns

Performance: ✅ Efficient - derived state only recomputes on dependency changes

Recommendation: The PR is safe to merge. The remaining issues are minor type accuracy and documentation improvements that don't affect functionality.

---

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

Theme defaults to light before system theme initializes

Medium Severity

The refactored theme logic causes a brief flash from light to dark theme on initial render for users with dark system preference. Previously, useTheme() in ThemeContext.tsx called useColorScheme() directly as a fallback when context.themeMode was undefined. Now, the derived themeMode in ThemeController defaults to 'light' when both defaultThemeMode and systemThemeMode are undefined, and setSystemThemeMode is only called in the modal's useEffect (after first render). Components like ConnectButton and AccountButton will initially render with light theme regardless of system preference until the modal mounts and sets the system theme.

Additional Locations (2)

• packages/appkit/src/modal/w3m-modal/index.tsx#L36-L39
• packages/core/src/controllers/ThemeController.ts#L21-L26

 

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud