Add legacy web renderer debugger bridge - #157
Conversation
📊 PR Size: size/XLTotal changes: 2183 lines (7 files) Top files changed:
Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
|
| Test Suite | Result |
|---|---|
| Snapshot Tests | ✅ success |
| Test Coverage Delta | ✅ success |
| Valdi Smoke Tests | ✅ success |
| API Surface Check | ✅ success |
| Linux: C++ Tests | ✅ success |
| Linux: Registry Validation | ✅ success |
| Linux: Build Compiler | ✅ success |
| Linux: Module Tests | ✅ success |
| Linux: Build & Export | ✅ success |
| valdi_web Integration Test | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
clholgat
left a comment
There was a problem hiding this comment.
Review of the legacy web renderer debugger bridge. The serialization is impressively defensive (bounded depth/entries/characters, getter-safe, liveness re-checks), and the bridge is correctly restricted to top-level windows via query flags with no eval / postMessage receiver / injection sink. One medium item is inline; one low is below.
NOTE: This 24-PR stack is being squashed into ~3 PRs — please carry this feedback into the squashed PR(s).
🟢 Low — ValdiWebRendererDelegate.ts onElementDestroyed (~L112-146): changed from single-element destroyElement to a full recursive subtree teardown on the production render path (not debug-gated), to keep backing-node parent/child pointers consistent for the debug traversal. It is idempotent and guarded, but it couples core teardown semantics to a debug-only requirement — a regression here would affect all web rendering, not just debugging. Worth a comment explaining why the subtree walk is required, plus test coverage for the non-debug destroy path.
|
|
||
| export interface StandaloneWebDebuggerRuntime { | ||
| clearHighlight?(): boolean; | ||
| getSnapshot(): StandaloneWebDebuggerSnapshot; |
There was a problem hiding this comment.
🟠 Med — This bridge exposes only getSnapshot/highlightNode and does not implement the unified ValdiDebuggerInput contract from the input-control PR (tap/focus/text/key/scroll). As a result valdi inspect input tap|text|key|scroll against a legacy-web-renderer target silently returns the "target app did not register the Valdi debugger input handler" fallback — input forwarding works on native but no-ops on web, with no shared abstraction between the two debugger surfaces.
Failure: users driving input through the CLI/panel get silent no-ops on web targets.
Suggested fix: at minimum document the read/highlight-only scope of the web bridge; ideally route web input through the same contract so the two surfaces don't diverge as the stack is unwound.
|
Superseded by #180, which consolidates this patch into the reviewed debugger capabilities landing unit. The replacement carries forward the feedback and fixes discussed here. Closing this draft to reduce the active stack; this PR and its discussion remain the historical review record. |
Description
Bridges the debugger to Snap's existing legacy web renderer without requiring the unrelated renderer/runtime rewrite.
Type of Change
Testing
bazel test //...)Testing Details
npm testpassed 436/436; the CLI production build passed.//src/valdi_modules/src/valdi/web_renderer:testpassed.bazel query //...passed.Checklist
Related Issues
Relates to #154
Additional Context
Stack 4/22. Stacked on #156 (
bjd/debugger-input). Review this PR as the single incremental commit6f465ec0against that base; do not merge it before its parent.