Add cross-platform debugger input control - #156
Conversation
📊 PR Size: size/XLTotal changes: 6692 lines (28 files) Top files changed:
...and 18 more files Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
|
| Test Suite | Result |
|---|---|
| Valdi Smoke Tests | ✅ success |
| API Surface Check | ✅ success |
| Linux: C++ Tests | ✅ success |
| Linux: Module Tests | ✅ success |
| Snapshot Tests | ✅ success |
| Test Coverage Delta | ✅ success |
| Linux: Registry Validation | ✅ success |
| Linux: Build Compiler | ✅ success |
| valdi_web Integration Test | ✅ success |
| macOS: C++ & Platform Tests | ❌ failure |
| Linux: Build & Export | ✅ success |
| Linux: Hotreload Smoke | ✅ 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
clholgat
left a comment
clholgat
left a comment
There was a problem hiding this comment.
Review of the cross-platform debugger input control. Overall this is solid: request/response validation is thorough, the debugger HTTP server enforces loopback Host/Origin/Sec-Fetch-Site checks, and the input handler is correctly gated behind runtime.isDebugEnabled. Two medium items are 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 — valdi/src/valdi/macos/Views/SCValdiMacOSTextField.m (~L235-236): inside textDidChange:, the onWillChange replacement path reassigns both self.stringValue = replacementText and editor.string = replacementText while the field editor is active. Reassigning stringValue mid-edit can disturb the field editor / insertion state; the following selectedRange set mostly compensates but is fragile relative to the iOS reference path it replaces. Prefer updating only the field editor's contents while editing.
| message: `Unsupported debugger input type '${request.type ?? ''}'.`, | ||
| }; | ||
| } catch (error) { | ||
| if (error instanceof DebuggerTraversalFailure) { |
There was a problem hiding this comment.
🟠 Med — This catch converts only DebuggerTraversalFailure into a structured result and rethrows everything else, but the target app's own event handlers (onTap, onChange, onWillChange, onReturn, onWillDelete) are invoked inside this same try block. Any exception they throw escapes as a rejected messageReceived promise.
Failure: a component whose onChange throws turns a debugger text/tap request into an unhandled rejection in the runtime and a 5s client-side timeout instead of a clean error response.
Suggested fix: wrap the app-callback invocations (or the whole dispatch) so app-thrown errors are returned as { handled: false, message } rather than propagating.
| const absoluteY = request.y === undefined ? position.y + tapTarget.element.frame.height / 2 : request.y; | ||
| const localX = absoluteX - position.x; | ||
| const localY = absoluteY - position.y; | ||
| tapTarget.callback({ |
There was a problem hiding this comment.
🟠 Med — tap invokes the resolved onTap unconditionally with no hit-test / visibility / bounds check, and the selector resolves against the entire render tree (collectElements from root, L533), so off-screen or occluded elements are reachable. Supplied x/y are passed straight through without validating they fall inside the element frame.
Failure: a debugger tap can fire a handler a real user could never reach — e.g. a destructive button covered by a modal, or an element scrolled off-screen — a scope escape beyond what a user can actually touch.
Suggested fix: validate the tap point against the element's absolute frame and skip elements that aren't hit-testable / visible.
clholgat
left a comment
clholgat
left a comment
There was a problem hiding this comment.
Review of the cross-platform debugger input control. Overall this is solid: request/response validation is thorough, the debugger HTTP server enforces loopback Host/Origin/Sec-Fetch-Site checks, and the input handler is correctly gated behind runtime.isDebugEnabled. Two medium items are 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 — valdi/src/valdi/macos/Views/SCValdiMacOSTextField.m (~L235-236): inside textDidChange:, the onWillChange replacement path reassigns both self.stringValue = replacementText and editor.string = replacementText while the field editor is active. Reassigning stringValue mid-edit can disturb the field editor / insertion state; the following selectedRange set mostly compensates but is fragile relative to the iOS reference path it replaces. Prefer updating only the field editor's contents while editing.
|
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. |
2 participants
Description
Adds bounded, target-scoped debugger input routing across the CLI, browser panel, runtime handler, and macOS text input path.
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 3/22. Stacked on #155 (
bjd/debugger-explicit-port). Review this PR as the single incremental commit7a4b9710against that base; do not merge it before its parent.