Skip to content

Add hierarchical color palettes and theme-aware assets#121

Open
scorsin-oai wants to merge 1 commit into
Snapchat:mainfrom
scorsin-oai:simon/260723-hierarchical-color-palettes
Open

Add hierarchical color palettes and theme-aware assets#121
scorsin-oai wants to merge 1 commit into
Snapchat:mainfrom
scorsin-oai:simon/260723-hierarchical-color-palettes

Conversation

@scorsin-oai

Copy link
Copy Markdown
Collaborator

Description

This change adds support for overriding colorPalette on a per node level. This allows to turn a subtree into light or dark mode, regardless of the system configuration. In addition, it adds support for themeable assets, which makes it possible for an asset to have a different variant depending on the resolved color palette. One can use this to make a dark and light asset, similar to how we can make an asset that has a direction version for LTR and RTL.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation improvement
  • Performance optimization
  • Test improvement
  • Other (please describe)

Testing

  • Tests pass locally (bazel test //...)
  • Added/updated tests for changes (if applicable)
  • Tested on multiple platforms (iOS/Android/Web/macOS as applicable)
  • Manual testing performed (describe below)

Testing Details

Checklist

  • Code follows project style guidelines
  • Documentation updated (if needed)
  • No breaking changes (or documented in description)
  • Commit messages follow conventional format
  • No secrets, API keys, or internal URLs included

Related Issues

Additional Context

@github-actions github-actions Bot added area/runtime Valdi runtime (C++/native) platform/ios iOS-specific platform/android Android-specific labels Jul 23, 2026
@github-actions

Copy link
Copy Markdown

Sensitive Files Detected

⚠️ Core runtime — App-wide blast radius — touches the rendering core. Must be gated.

This is an automated notice. A maintainer will review after import.

@github-actions

Copy link
Copy Markdown

🎉 Bazel & CI Test Results

Test Suite Result
API Surface Check ✅ success
Linux: Build & Export ✅ success
Linux: C++ Tests ✅ success
Linux: Build Compiler ✅ success
Valdi Smoke Tests ✅ success
Snapshot Tests ✅ success
macOS: C++ & Platform Tests ✅ success

All Bazel configuration and CI tests passed!

The build system and core tooling are working correctly.

🚀 Bazel remote cache is now enabled - future builds will be faster!

Workflow: Valdi CI

@scottthompsonsc

Copy link
Copy Markdown
Contributor

Thanks for the contribution! While bringing this into Snapchat's build a few issues came up in review that are worth addressing here. I've verified each against the code and grouped them by confidence. References are by function name.

Confirmed functional bugs

1. boxShadow: 'none' no longer clears the shadow (runtime/Attributes/DefaultAttributeProcessors.cpp)

preprocessBoxShadow returns Value::undefined() for "none", but the new postprocessBoxShadow(ViewNode&, const Value&) calls resolveColorAtIndexInArray(...) with no array guard, and that helper returns Error("Invalid array value") on a non-array input. So setting boxShadow: 'none' now fails postprocessing and the old shadow stays on the view. postprocessGradient(ViewNode&, ...) already guards with if (!in.isArray()) return in;. Same guard is needed here:

Result<Value> postprocessBoxShadow(ViewNode& viewNode, const Value& in) {
    if (!in.isArray()) {
        return in;
    }
    constexpr size_t kBoxShadowColorIndex = 4;
    auto resolvedBoxShadow = resolveColorAtIndexInArray(viewNode, in, kBoxShadowColorIndex);
    ...
}

2. configureColorPalette stores keyword/alias colors as transparent black (runtime/Runtime.cpp)

The new path parses each value with ValueConverter::toColorValue(...) then does Color(colorValue.value().toLong()). For hex / rgba(...) this yields a numeric Value and works. But for a color keyword or alias (for example red, or a semantic alias name), parseColorValue returns a string Value, and Value::toLong() on a non-numeric string goes through atoll() and yields 0, so the color is silently stored as rgba(0,0,0,0). The previous ValueConverter::toColor(*colorPalette, ...) resolved aliases and returned an error on failure instead of silently writing transparent black. Recommend either resolving the alias against the palette being configured / the default palette, or emitting an error when !colorValue.value().isNumber() rather than saving 0. (Snapchat's own palettes are all hex literals so we don't hit this internally, but it is a real behavior change for the framework.)

Thread-safety (please sanity-check against Valdi's threading model, you know it best)

The multi-palette design introduces cross-thread access patterns that the single fixed palette did not have. Writes are dispatched to the main thread (runtimeConfigureColorPalette / runtimeSetActiveColorPalette via dispatchOnMainThread), while reads happen from JS-thread paths (AttributedTextNativeModuleFactory::makeNativeAttributedText, and ViewNodeRenderer::visit / ViewNode::setColorPaletteName depending on tree thread affinity). ColorPaletteManager and ColorPalette have no internal synchronization. Two hazards that are genuinely new versus the old code:

  • Active-palette reassignment race: setActiveColorPalette reassigns the _activeColorPalette Ref on the main thread while getActiveColorPalette() reads/copies it on the JS thread. The old code never swapped the palette pointer (it only mutated one fixed palette's contents), so the Ref control-block/refcount race is new and can lead to use-after-free.
  • Lazy insertion on the read path: getColorPalette() calls getOrCreateColorPalette(), which inserts into _colorPaletteByName (a phmap::flat_hash_map) when the name is missing. A "read" that mutates the map can race with configureColorPalette and reallocate the backing store.

If Valdi guarantees these accesses are serialized onto one thread, this is a non-issue and can be dismissed. If not, the fix is a mutex in ColorPaletteManager (and ColorPalette), with getActiveColorPalette() / getColorPalette() returning Ref<ColorPalette> by value so the refcount is bumped under the lock.

Defensive / robustness (lower urgency)

3. RuntimeManager never unsets its listener (runtime/RuntimeManager.cpp)

The ctor does _colorPaletteManager->setListener(this), but neither ~RuntimeManager() nor fullTeardown() calls setListener(nullptr). Because each Runtime holds a strong Ref<ColorPaletteManager>, the manager can outlive the RuntimeManager; a subsequent configureColorPalette / setActiveColorPalette on a surviving Runtime would call onColorPaletteManagerUpdated on the freed RuntimeManager. Add setListener(nullptr) on teardown.

4. ViewNode::setColorPaletteName can null-deref (runtime/Context/ViewNode.cpp)

In the non-empty-name branch, SC_ASSERT(_viewNodeTree != nullptr, ...) compiles out in release, and _viewNodeTree->getViewManagerContext() can itself return null (the parallel getParentResolvedColorPalette guards both; this branch does not). Suggested:

if (_viewNodeTree != nullptr) {
    if (const auto& viewManagerContext = _viewNodeTree->getViewManagerContext()) {
        colorPalette =
            viewManagerContext->getAttributesManager().getColorPaletteManager()->getColorPalette(colorPaletteName);
    }
}
setHasOveriddenColorPalette(true);

5. ViewNode::insertChildAt can leave a child with a stale palette (runtime/Context/ViewNode.cpp)

The if (_colorPalette != nullptr) guard around child->setInheritedColorPalette(...) means a child moved under a parent whose resolved palette is null keeps its previous (stale) palette instead of being reset. setInheritedColorPalette already handles nullptr safely (clears + flushes to defaults), so the guard can be dropped:

child->setInheritedColorPalette(viewTransactionScope, _colorPalette);

(This is largely theoretical since in-tree nodes normally resolve to the non-null active palette, but the invariant is cleaner without the guard.)

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

Labels

area/runtime Valdi runtime (C++/native) platform/android Android-specific platform/ios iOS-specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants