Skip to content

Add CollapseWebPaths test: transitive-dependency res must be registered - #153

Open
beaucollins wants to merge 1 commit into
simon/260814-valdi-webfrom
beaucollins/web-transitive-res-registry
Open

Add CollapseWebPaths test: transitive-dependency res must be registered#153
beaucollins wants to merge 1 commit into
simon/260814-valdi-webfrom
beaucollins/web-transitive-res-registry

Conversation

@beaucollins

Copy link
Copy Markdown
Collaborator

Adds a CollapseWebPaths unit test that documents a regression on this branch (#148): transitive-dependency res/ images are copied into the web package but never registered in _image_registry.js, so getAssets() can't resolve them at runtime and the icons render blank.

Root cause

valdi/compiler/toolbox/src/valdi/compiler_toolbox/CollapseWebPaths.cpp generateImageRegistry (L274) scans only the immediate children of src/ for <module>/res (DiskUtils::listDirectory(sourceDirectory), L276). The collapse stages direct-dep res at top-level src/<module>/res (registered) but transitive-dep res at a nested src/release/res/<module>/res (never scanned). Observed on a real package: only the direct module appeared in _image_registry.js; ~20 coreui icons and other transitive res were bundled but unregistered.

Repro

TEST(CollapseWebPaths, registersTransitiveDependencyResources) builds a package with a direct-dep res and a transitive-dep res (staged under release/res/shared/res) and asserts both appear in the registry. It fails on this branch because the transitive entry is dropped.

Suggested fix

Enumerate res under the nested release/res/<module>/res locations too (or stage all module res uniformly at src/<module>/res), so the registry matches the full transitive web-dep closure.

Transitive-dependency res is staged under a nested `release/res/<module>/res`
path, but generateImageRegistry only scans the immediate children of `src/`
(DiskUtils::listDirectory(sourceDirectory)). Transitive modules' images are
copied into the package yet never added to `_image_registry.js`, so getAssets()
resolves them to nothing at runtime and the icons render blank. Only the direct
dependency (whose res sits at top-level `src/<module>/res`) is registered.

This test builds a package with a direct-dep res and a transitive-dep res (staged
under `release/res/shared/res`) and asserts both appear in the registry. It fails
on the current renderer because the transitive entry is dropped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

📊 PR Size: size/S

Total changes: 39 lines (1 files)

Top files changed:

  • valdi/compiler/toolbox/test/CollapseWebPaths_tests.cpp: +39 -0

Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+)

@github-actions

Copy link
Copy Markdown

⚠️ Bazel & CI Test Results

Test Suite Result
API Surface Check ✅ success
Linux: Module Tests ✅ success
valdi_web Integration Test ❌ failure
Snapshot Tests ✅ success
Valdi Smoke Tests ✅ success
Linux: Build & Export ✅ success
Linux: C++ Tests ❌ failure
Linux: Hotreload Smoke ✅ success
Linux: Build Compiler ✅ success
macOS: C++ & Platform Tests ✅ success
Linux: Registry Validation ✅ success

Some tests failed. Please check the workflow logs for details.

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

Workflow: Valdi CI

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.

1 participant