Repository navigation
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed all six changed files at 3c93b4b17c17fdccc6b62761346f3d41424663d3, the RichEditor input sink and the corresponding keyboard, responder, pointer-picking and Tab paths.
No confirmed P0–P2 finding in this source review. The patch separates keyboard focus (can_focus) from pointer/accessibility targeting (can_reach), permits the box-none sink to retain keyboard focus, and prevents its hosted native subview from intercepting pointer hits. It also filters disabled/nonfocusable/implicitly read-only inputs during Tab traversal and routes modified paste to the native plain-text action. I checked 20 controlled ancestry/state cases against the actual extracted methods; those use simulated native-object responses and are not AppKit acceptance.
Current macOS job 128656562911 passes 86 platform tests and all five surface tests. The expanded hidden/clipped-subtree case (0.513s) includes pointer pass-through to the covered control, programmatic sink focus, focus retention across a layout update, paste, forward/backward Tab and stale composition-target revocation. This is broader coverage than the already-merged #6706's narrow input-focus fix; the two patches should not be treated as wholly equivalent.
The macOS desktop suite gets past the formerly failing first input and passes three cases, then fails the fourth case after 65.856s: the expected selected.png draft does not appear in the editor (observed: Null, desktop_new_session.rs:265). Eighteen cases remain unrun due to fail-fast. Separately, Linux CLI shard job 128658882365 fails model_and_source_search_preserves_focus_and_keyboard_selection (67.777s, keyboard.rs:315); I have not established causality for that unchanged TUI path. The overall run 42456459549 is failure, not a full desktop pass. Native Shift-modified paste is source-checked here, not independently exercised locally.
Integration: an actual merge into current rust-preview 74765152 exits 1 with a content conflict in nsh-platform-macos/src/view.rs, where #6706 changed the same reachability logic. Resolve that overlap while retaining the additional behavior and tests. I did not run AppKit or real IME locally. No schema or wire changes.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
3c93b4b to
06548f7
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed all 11 changed files at 06548f7a8d991f003856e15671788717a4810ecb, including the rebased focus changes, the new per-surface clipboard and native-editor shortcuts, their terminal-output consumer, and the model-search test synchronization. The earlier 3c93b4b1 review applies to the previous revision; this revision merges without conflicts into current rust-preview 74765152.
No confirmed P0–P2 finding in this source review. Surface::terminal_clipboard now separates the locally readable text from a pending write consumed once by the terminal output path. system.rs::write_clipboard, text.rs::copy and the terminal native-editor copy/cut path use that shared surface state; setting incoming clipboard text does not echo a write or modify the global pasteboard. The ordinary nonterminal clipboard path still uses AppKit's general pasteboard. The direct shortcut handling checks secure-copy and editability policy, bounds the selected UTF-16 range, and delegates replacement/paste and undo to the existing editor paths. I also read the new controlled two-surface native test, including single-line/multiline paste, cut/undo/redo and masked-input copy suppression.
I compiled and ran the actual extracted clipboard write/read/set/take methods with controlled OS-pasteboard and event adapters. Per-surface isolation, one-shot/latest pending writes, incoming-text no-echo behavior, empty and Unicode text, has-only reads, the ordinary OS branch and the unset-local fallback pass. The actual focus predicate bodies are unchanged from the previous revision and pass the same 20 controlled ancestry/state cases. These checks do not execute AppKit, native shortcuts, undo, IME or real clipboard ownership.
keyboard.rs now waits for the narrowed 1 model search result before choosing the previously visible model row. That directly addresses the stale-row window in the old test; the author's eight stress repetitions remain author-reported. I did not rerun the real Host/TUI test locally.
Current CI does not establish the native acceptance claimed in the PR description. Run 42465390034 is failure: macOS job 128686791569 stops during setup because downloading cargo-binstall returns HTTP 504 (exit 56), before platform or desktop tests run. The root-build, packaging and Windows jobs fail while downloading the v8 150.4.0 prebuilt archive, also with HTTP 504. The Cargo manifests/lockfile and CI setup action are unchanged from 74765152; these logs do not establish a PR-caused code failure. They also provide no current-head native or desktop pass. No rerun was triggered.
For the separate current-base result, 74765152 run 42460736390 is now failure: macOS passes 86 platform tests and six surface tests, then three desktop cases; the fourth fails after 67.967s because the expected selected.png draft is absent (desktop_new_session.rs:265), leaving 18 cases unrun. This is baseline evidence, not a result for the new clipboard implementation. The previous head's surface passes and the author's stated native/22-desktop acceptance cannot substitute for current-head execution. No schema or wire changes.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
ed8e794 to
465e1da
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed the update to 465e1da7c8f6c11b896ce62d5972a77a34f6c42b. The new commit changes three CI files: .github/actions/ci-setup/action.yml, scripts/ci/setup.mjs and scripts/ci.test.mjs. All 11 runtime/test files covered in my previous revision review retain identical Git blobs. An actual merge into current rust-preview 74765152 still succeeds without conflicts.
No confirmed new P0–P2 finding in this update. Rust-tool installation now uses GitHub release metadata and the API asset endpoint instead of cargo-binstall's web-download path. The downloader retries transient HTTP failures, rejects mismatched SHA-256 values when the release metadata supplies a digest, and installs through a temporary file. The new V8 setup selects the locked crate version and host target, fills the compressed Cargo cache, and stages the debug-profile static library plus its URL marker before compilation. The action also stages it for packaging workers whose compiler-cache option is disabled.
I inspected the corresponding upstream V8 150.4.0 build script: its compressed-cache branch copies the archive and returns before the normal download branch writes the .sum URL marker. That supports the marker-handling premise. I did not independently reproduce the author's concurrent Rust-link truncation, and the staging helper's explicit target/debug path is not evidence for arbitrary Cargo profiles or cross-target builds.
Executed checks: I ran the new unmodified loopback-HTTP test under Node 24.18.1 (1 passed, 0 failed). It exercises an authenticated API request, a transient 504 retry, corrupted-digest rejection before installation, cold V8 cache/library/marker creation, repeated setup without another request, and recovery of a missing gn_out from the compressed cache. These use fixture archives, not real native binaries. The current hosted tooling job 128709991162 separately reports 265 passed, 0 failed, 1 skipped. Root-build 128709991182 and packaging 128709991218 both succeed; their raw logs show the new V8 staging step, and root-build also records successful nextest installation.
The previously failing real Host/TUI model-search case now passes in 11.684s in current-head CLI shard job 128713108782 (19 passed, 93 skipped). This is current execution evidence; the earlier old-head keyboard failure should not remain a current failure claim.
Acceptance boundary: run 42473582157 was still running when inspected, with native-macos and native-windows unfinished. I have no current-head native clipboard/undo/IME or complete desktop-suite result yet, and did not run AppKit locally. The previous 06548f7a download-504 result and 74765152's fourth desktop draft failure remain historical/baseline evidence, not this head's terminal result. The PR description's full-matrix claim therefore remains unconfirmed at inspection. No schema or wire changes.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Separate keyboard focus reachability (NshView::can_focus) from pointer hit-targeting (NshView::can_reach) so RichEditor's pointerEvents="box-none" input sink can accept and retain AppKit first-responder focus while pointer hits pass through its hosted InputPad, and skip non-editable inputs during Tab traversal. Generated-by: Antigravity
Generated-by: Antigravity
465e1da to
e7a17f7
Compare
Generated-by: Antigravity
e7a17f7 to
0b8c5fe
Compare
There was a problem hiding this comment.
I reviewed the update to 0b8c5fead417971c34df9d145de292c6872c0011: four files change relative to the previously reviewed 465e1da7. The other ten changed-file blobs are identical. An actual merge into current rust-preview 74765152 succeeds without conflicts.
No confirmed new P0–P2 finding in this update. The previous revision introduced a real CI cleanup failure: both native jobs rejected the precreated Cargo target directory because it lacked CACHEDIR.TAG. setup.mjs now writes Cargo's standard signature before staging V8, also repairs a missing tag on the already-staged and prune paths, and uses the same fixed target directory as its prune command. I ran the actual old and new setup scripts against temporary, otherwise equivalent Cargo fixtures: old setup → cargo clean exit 101 / missing tag; new setup → exit 0. This uses actual Cargo cleanup with a fixture compressed library, not a native build. The updated unmodified loopback API/tag test also passes locally under Node 24.18.1 (1 passed, 0 failed).
The native test's submit handler now reads {text}, matching the actual Input event declaration in nsh-core/src/js/mod.rs, rather than the nonexistent {value} field. Extracting the two actual handlers and invoking the documented payload produces submitted:undefined before and the expected submitted string after. This controlled check does not locate the precise wait that failed in the previous AppKit run. The three geometry helpers newly gated by feature = "terminal" are used by that same feature's terminal paths; their computation is unchanged.
Current macOS job 128734083421 passes 86 platform tests and all seven surface tests, including native_input_shortcuts_copy_cut_paste_and_undo_without_global_pasteboard (0.356s). The previous revision's new clipboard test failed; that is no longer the current surface result. Current macOS cache pruning also succeeds, recording removal of 2,623 files, rather than the former missing-tag error. Current tooling reports 265 passed, 0 failed, 1 skipped, and the real Host/TUI model-search case passes again in 11.644s in CLI job 128737067968 (19 passed, 93 skipped).
The complete desktop fix is still unconfirmed. This head's macOS desktop suite passes its first three cases, then fails desktop_creates_only_on_first_submission_and_preserves_the_confirmed_snapshot after 66.104s: the expected selected.png draft is absent (desktop_new_session.rs:265, observed: Null). Eighteen cases remain unrun due to fail-fast. The current base has the same failing draft wait; I have not established this update as its cause. Surface coverage does not establish complete desktop acceptance.
Run 42481390738 has now completed with failure, with macOS as its only failing platform job. Current-head Windows job 128734083388 is success: its raw log records 22/22 desktop tests passed (327.281s), including the image-draft case (14.199s), and successful cache pruning. Windows acceptance and the Cargo-tag repair are therefore confirmed for this head; the macOS desktop failure above still prevents complete cross-platform acceptance. I did not run AppKit, a real macOS clipboard or IME locally. No schema or wire changes.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Summary
Fixes the remaining
native-macosmaka-cli::native-desktop(desktop_new_session),root-tests(keyboard.rs), and GitHub Releases504/ parallelv8build-script CI failures onrust-preview:SurfaceTerminalClipboardand handle nativeInput(FieldEditor/TextArea) shortcuts directly on macOSTerminalSurface:NSPasteboard::generalPasteboard().changeCount()polling and duplicateclipboard/terminal_clipboardslots with per-SurfaceTerminalClipboard { text, pending }(matchingnsh-platform-windows), soCommand::SetClipboard,text::copy, and native editor copy/cut populatepending+textwhileset_clipboard_textonly populatestextwithout touching the shared OS pasteboard.Ctrl/Cmd+A,Ctrl/Cmd+C,Ctrl/Cmd+X,Ctrl/Cmd+V,Ctrl/Cmd+Z,Ctrl/Cmd+Shift+Z, andCtrl/Cmd+Ydirectly against the focusedNSTextView(FieldEditororTextArea) instead of relying on[NSTextView copy:nil]/[NSTextView cut:nil]andNSPasteboard::generalPasteboard()in unactivated headlessTerminalSurfacewindows.setAllowsUndo(true)onFieldEditorandFieldCell, preservesetSelectable(true)acrossEditor::Lineeditableupdates (matchingEditor::Area), and acceptCtrl+A/Ctrl+Cintext::selection_keywhensurface.terminalis active.NshView::can_focus) from pointer hit-targeting (NshView::can_reach) and pass hits throughPointerEvents::BoxNonehosted views:on_nativeinpick_ingated onpe != PointerEvents::BoxNoneso clicks atRichEditor's caret pass through the1x16sink's hostedInputPad, and alignTabtraversal to skipdisabled,focusable={false}, andreadonlyinputs.model_and_source_search_preserves_focus_and_keyboard_selection(keyboard.rs):"1 model"after pasting"fixture-model-alt"sowait_enabled_button("Use fixture-model-alt · NSH fixture")does not match the stale row from the preceding 2-model search result before asynchronous model results arrive.rusty_v8archives via authenticated GitHub REST API (api.github.com):taiki-e/install-action(cargo-binstall) and fetchcargo-nextest,cargo-deny, anddenoland/rusty_v8release assets via authenticatedapi.github.com(Accept: application/octet-stream) withsha256digest verification and retry backoff, eliminating unauthenticatedgithub.com/.../releases/download/...504failures across cold caches andpackaging(compile: false).target/debug/gn_out/obj/{librusty_v8.a,rusty_v8.lib}, atomically write{librusty_v8.sum,rusty_v8.sum}alongside~/.cargo/.rusty_v8, and write Cargo'sCACHEDIR.TAGwhen pre-creatingtarget/, preventing parallel host/targetv8build scripts from truncating the archive while keepingcargo cleaninci-cachefunctional on cold caches.Refs #6698, #6706
Verification
cargo nextest run -p maka-cli --test integration -E 'test(model_and_source_search_preserves_focus_and_keyboard_selection)' --stress-count 5(and--stress-count 3) — 8/8 passednode scripts/ci.mjs check format— passednode scripts/ci.mjs check licenses— passednode scripts/ci.mjs check notices— passednode scripts/ci.mjs check tooling-tests— 265/265 passed (including loopback HTTP API504retry,sha256mismatch rejection, and idempotent V8.rusty_v8+gn_out/obj.sum+CACHEDIR.TAGstaging tests, plus liveapi.github.comsmoke test)native-macos,native-windows,native-ubuntu,linux-nsh,nsh,termina,root-tests,packaging,product,bridge,tooling) verified before merge.AI use
Select exactly one:
Tool(s) and scope: Antigravity diagnosed the headless macOS
TerminalSurfaceFieldEditorcopy/pasteboard failure,PointerEvents::BoxNonehit/focus separation,keyboard.rsmodel search race, unauthenticated GitHub web release504failures, and parallelv8build-script archive truncation in CI setup; implemented the fixes and tests; and verified local and CI suites.Checklist
Does this PR entail a change in behavior?