Velox FFI adoption: Phases 1-4 complete#1
Conversation
Adopted Velox's runtime-wry-ffi architecture with the following changes: - Renamed all velox_* functions to wry_* - Split single lib.rs into modular files: - types.rs: FFI structs (handles, configs, enums) - helpers.rs: Utility functions (strings, panic guards, conversions) - app.rs: Event loop creation, proxy, pump - window.rs: Window creation + 50+ property operations - webview.rs: Webview creation, navigation, scripts - dialogs.rs: File/message dialogs (rfd, tinyfiledialogs) - events.rs: Event serialization to JSON - menu.rs: Native menus (macOS only) - tray.rs: System tray (macOS only) Key architecture changes: - Separate WryWindowHandle and WryWebviewHandle (was combined WindowState) - Event loop callback receives JSON-serialized events - Custom protocol handler receives full HTTP info (method, headers, body) New dependencies: rfd, tinyfiledialogs, muda, tray-icon, serde_json, url Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update WryInterop.cs with new P/Invoke declarations for Velox FFI - Update WryTypes.cs with structs/enums matching new Rust types - Update WryDelegates.cs with event loop and protocol handler callbacks - Replace WryAppHandle with WryEventLoopHandle/WryEventLoopProxyHandle - Split WryWindowHandle into WryNativeWindowHandle/WryNativeWebviewHandle - Implement event loop pump with JSON event dispatch in TauriWindow.Wry.cs - Add helper methods in TauriWindow.WryCompat.cs bridging API to new FFI - Update TauriWindow.cs properties to use new helper methods Key architecture changes: - Event loop: single pump with JSON callback instead of typed callbacks - Window/Webview: created separately instead of combined - Protocol handlers: new request/response structs with full HTTP info Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add WryIpcHandler type and wire up with_ipc_handler in webview builder - Add rwh_06 feature to Linux tao dependency for raw window handle support - Add IpcHandlerState to manage callback lifetime in C# - All 14 runtime tests pass: window, protocol, IPC, events Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
On Linux, WebKitGTK requires integration with GTK's widget hierarchy via build_gtk(vbox) instead of raw X11 window handles via build(window). The old wry-ffi used this approach; the Velox migration lost it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TauriCSharp.Dialogs static class with public API for file/message dialogs - Switch message dialogs from rfd to tinyfiledialogs (rfd fails silently on Linux) - Add dialog test buttons to test app UI (Open File, Save File, Confirm) - File dialogs still use rfd (works correctly with GTK file chooser) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Mark Phase 3 complete, Phase 4 in progress in roadmap - Update TauriCSharp README with current features and status - Rewrite wry-ffi README with accurate API examples - Add Linux GTK notes to architecture doc - Remove outdated GDK_BACKEND=x11 advice (causes cursor issues on WSL2) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make string fields in TauriNativeParameters nullable - Fix nullable return types for MacOsVersion, IconFile, Parent, UserAgent - Make constructor parameter nullable (TauriWindow parent) - Make dialog method parameters nullable (defaultPath, filters) - Fix TauriIpcException correlationId parameter - Use EventArgs.Empty instead of null in event invocations - Make CustomSchemes dictionary value type nullable - Lower default LogVerbosity from 2 to 0 (no logging by default) Remaining warnings are property getters returning nullable fields with non-nullable return types - can be addressed incrementally. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace custom LogVerbosity system with Microsoft.Extensions.Logging: - Add TauriLog.cs with [LoggerMessage] source-generated methods - Add ILogger parameter to TauriWindow constructor - Mark LogVerbosity/SetLogVerbosity as [Obsolete] - Replace critical Log() calls with typed TauriLog methods - Update TestApp to use LoggerFactory with console output Benefits: zero allocation when disabled, compile-time validation, structured logging support, standard .NET logging pattern. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Make string and event handler property return types nullable to match their underlying nullable backing fields, preventing callers from getting NullReferenceException at runtime on genuinely optional values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add [assembly: DisableRuntimeMarshalling] since all native interop goes through the Rust wry-ffi layer (1-byte bool). Migrate all 10 remaining DllImports to LibraryImport, remove [MarshalAs] from struct fields, and fix CA1420/CA1421 warnings: - Remove [UnmanagedFunctionPointer] from legacy Cpp* delegates (purely C#-side callbacks, no longer used for native interop) - Replace Marshal.PtrToStructure/StructureToPtr with Unsafe.Read/Write for blittable FFI structs - Replace Marshal.SizeOf with Unsafe.SizeOf - Fix IDE0060 unused params, CA1822 static suggestions, remove dead GetNativeFilters method - Modernize: primary constructors, collection expressions, TryGetValue, range indexing, GC.SuppressFinalize in Dispose Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add wry_webview_open_devtools and wry_webview_close_devtools FFI exports in Rust, LibraryImport bindings in C#, and update the TauriWindow stubs to call through to native wry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On Linux, tao sends all window property changes through an async channel to the GTK event loop. Native getters read current GTK state synchronously, so an immediate read-after-write returns stale values. Switch to shadow state pattern: property getters read from _startupParameters, setters always update shadow before calling native, and event callbacks sync shadow on external changes (user resize, drag, minimize button, etc.). Properties fixed: Title, FullScreen, Maximized, Minimized, Size, Location. Event callbacks updated: OnSizeChanged, OnLocationChanged, OnMaximized, OnMinimized, OnRestored. Removed 6 dead native getter methods from WryCompat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add monitor/display info (4.4), window icons (4.5), desktop notifications (4.6), global shortcuts (4.7), and multi-window support (4.8) across Rust FFI and C# layers. Rust: Add image, notify-rust, global-hotkey crates. New modules for notifications and shortcuts. Icon loading from file/RGBA, hotkey event polling in event loop pump. C#: Wire monitor properties to existing FFI. Add Notifications, GlobalShortcuts, and TauriApp static classes. Route child window events through TauriApp when main window drives the event loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rust: Extend WryWindowConfig with parent handle and modal flag. Use platform-specific APIs (transient_for+set_modal on GTK, owner_window+set_enable on Windows, parent_window on macOS). Add wry_window_set_enabled for modal parent disable/restore. C#: SetParent() and SetModal() fluent API on TauriWindow. Modal parent auto-restores on child close. TauriApp gains SendToWindow(), Broadcast(), GetWindow(), and Windows for cross-window messaging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expand key components, add multi-window architecture, FFI conventions, and platform feature table. Update status to Phase 4 complete, Phase 5 next. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Self-drives 19 tests through the real IPC path (ExecuteScript → JS sendMessage → Rust → C# handler) with a chained autorun-next pattern. Exits 0 on pass, 1 on fail, 2 on 30s timeout. Skips 5 tests requiring user interaction (dialogs, modal, navigation). Also adds RecordTest calls to 8 handlers that were missing test coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces an app-centric native bridge with an event-loop–centric wry-ffi backend and broad C#/Rust integration: new Rust FFI modules (event loop, windows, webviews, dialogs, menus/tray, notifications, shortcuts), major C# interop and API surface changes (handles, P/Invoke, Dialogs/Notifications/GlobalShortcuts, TauriApp), docs, test autorun, logging, and build tooling. Changes
Sequence DiagramsequenceDiagram
participant TestApp as TestApp (C#)
participant TauriApp as TauriApp (C#)
participant WryInterop as WryInterop (P/Invoke)
participant WryFFI as wry-ffi (Rust)
participant OS as OS/WebView
TestApp->>TauriApp: Create window / send autorun command
TauriApp->>WryInterop: EventLoopNew / WindowBuild / WebviewBuild
WryInterop->>WryFFI: wry_event_loop_new / wry_window_build / wry_webview_build
WryFFI->>OS: create native event loop, window, webview
OS-->>WryFFI: native events (input/window/shortcut)
WryFFI->>WryInterop: serialize event JSON (event loop callback)
WryInterop->>TauriApp: WryEventLoopCallback (event JSON)
TauriApp->>TestApp: dispatch to window / IPC
TestApp->>TauriApp: sendMessage / dialog request
TauriApp->>WryInterop: Dialog/Notification/Shortcut calls
WryInterop->>WryFFI: invoke corresponding FFI functions
WryFFI->>OS: show dialog/notification or register shortcut
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude review this |
Rust FFI expects UTF-8 (*const c_char via CStr), but Dialogs.cs was using StringToHGlobalAnsi/PtrToStringAnsi which would corrupt non-ASCII characters. Switch to StringToCoTaskMemUTF8/PtrToStringUTF8 with matching FreeCoTaskMem, consistent with the rest of the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…leaks - helpers.rs: Fully qualify std::ptr::null_mut() in macOS-only guard_panic<T> to fix compilation failure on macOS - TauriApp: Track pending windows from CreateWindow(), initialize them in Run() before the emptiness check so multi-window mode actually works - webview: Store identifier CString in WryWebviewHandle at creation time, return pointer to stored field instead of leaking via CString::into_raw() - TauriWindow.Wry: Replace per-request delegate + GCHandle allocation with a single static delegate/function pointer for FreeResponseData Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/TauriCSharp/TauriCSharp/TauriNativeParameters.cs (3)
86-89:⚠️ Potential issue | 🟡 MinorStale TODO: this PR is the wry-ffi switch.
The comment says "Remove this limit when switching to wry-ffi," but this PR implements that switch. Should the 16-element fixed array be removed or the TODO updated?
170-183:⚠️ Potential issue | 🟡 MinorIncomplete and duplicated doc comments.
Several fields have placeholder
???descriptions (lines 170, 173, 176, 179, 182) and duplicated "Default is Default is" text (lines 164, 167, 176, 179). Also, line 105 has a truncated "Default is." sentence. These should be filled in or cleaned up while touching this file.
204-204:⚠️ Potential issue | 🟡 MinorTypo: "naviage" → "navigate".
Proposed fix
- response.Add("An initial URL or HTML string must be supplied in StartUrl or StartString for the browser control to naviage to."); + response.Add("An initial URL or HTML string must be supplied in StartUrl or StartString for the browser control to navigate to.");src/TauriCSharp/TauriCSharp.TestApp/Program.cs (1)
773-786: 🛠️ Refactor suggestion | 🟠 Major
RecordTestdeduplication viaContainsis fragile and O(n).
_testResults.ToString().Contains(testName)performs a full string scan and can produce false-positive matches when one test name is a substring of another (e.g., a test named"Restore window"would suppress a later test named"Restore"). Consider using aHashSet<string>for O(1) deduplication with exact matches.♻️ Suggested fix
Add a field:
+ private static readonly HashSet<string> _recordedTests = new();Then update
RecordTest:static void RecordTest(string testName, bool passed) { - // Only record each test once - if (_testResults.ToString().Contains(testName)) return; + // Only record each test once + if (!_recordedTests.Add(testName)) return;docs/05-roadmap.md (1)
224-237:⚠️ Potential issue | 🟡 MinorTask numbering mismatch: Phase 6 tasks are numbered 5.x, Phase 7 tasks are numbered 6.x.
Lines 224/229/234 use 5.1/5.2/5.3 under Phase 6, and Lines 253/258 use 6.1/6.2 under Phase 7. They should be 6.1/6.2/6.3 and 7.1/7.2 respectively.
src/TauriCSharp/TauriCSharp/TauriWindow.cs (1)
623-643:⚠️ Potential issue | 🟡 MinorAllow clearing
IconFilewhen set to null.
IconFileis nullable butFile.Exists(value)causes null to be treated as “missing,” throwing when callers attempt to clear the icon.🧭 Suggested handling for null/empty
set { if (_iconFile != value) { + if (string.IsNullOrEmpty(value)) + { + _iconFile = null; + if (_nativeInstance == IntPtr.Zero) + _startupParameters.WindowIconFile = null; + else + ClearIcon(); + return; + } if (!File.Exists(value)) { var absolutePath = $"{System.AppContext.BaseDirectory}{value}"; if (!File.Exists(absolutePath)) throw new ArgumentException($"Icon file: {value} does not exist."); }
🤖 Fix all issues with AI agents
In `@src/TauriCSharp/TauriCSharp/Interop/WryInterop.cs`:
- Around line 528-533: The WebviewIdentifier ownership docs conflict with
StringFree's no-op and WindowIdentifier's "do not free" wording; either change
WebviewIdentifier's XML comment to state the returned string is an internal
buffer that must not be freed (matching WindowIdentifier and StringFree), or
implement a proper P/Invoke wry_string_free and make StringFree call it so the
original "caller must free with wry_string_free" contract holds — pick one
approach and update the docstrings and implementation accordingly (modify
WebviewIdentifier and StringFree declarations), and also change the
CallConvCdecl usage at the other declaration to the fully-qualified
typeof(System.Runtime.CompilerServices.CallConvCdecl) for consistency.
In `@src/TauriCSharp/TauriCSharp/TauriApp.cs`:
- Around line 29-55: The singleton's Lazy<TauriApp> (_instance) always calls new
TauriApp() so the constructor's ILogger? parameter is never used and _logger
remains null; add a static Initialize(ILogger logger) method and make the
instance logger writable (remove readonly on _logger or add an internal
SetLogger method) so callers can provide an ILogger before first use (Initialize
should call Instance and set Instance._logger or call an internal SetLogger on
the singleton), or alternatively remove the logger parameter from the private
TauriApp constructor and the _logger field entirely if you choose to drop
per-app logging; update references to _logger accordingly (e.g., in LogError
calls).
- Around line 98-102: The field _shouldQuit is written by Quit() from other
threads and read by HandleEventLoopEvent on the event loop thread, so make its
updates visible across threads by changing its declaration to a volatile bool
(or use Interlocked/Thread.MemoryBarrier) and update any reads/writes
accordingly; specifically modify the _shouldQuit field declaration to be
volatile and keep Quit() calling TauriWindow.EventLoopProxyStatic?.RequestExit()
as-is so the event loop thread reliably observes the updated value in
HandleEventLoopEvent.
In `@src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs`:
- Around line 398-437: The protocol response currently leaks the MIME allocation
because mimePtr (created with Marshal.StringToCoTaskMemUTF8) is never freed;
update the response construction and FreeResponseData to free both the body and
the mime pointer by packaging both native pointers into a single native
user-data block: allocate a small native buffer (e.g., via Marshal.AllocHGlobal)
that stores the bodyPtr and mimePtr, set response.UserData to that buffer
instead of bodyPtr, and update FreeResponseData to read both pointers from that
buffer, call Marshal.FreeHGlobal on the body pointer and Marshal.FreeCoTaskMem
on the mime pointer, then free the user-data buffer itself; adjust the code in
the method that creates the WryCustomProtocolResponse (where bodyPtr, mimePtr,
response and UserData are set) and the FreeResponseData method so both pointers
are properly freed.
In `@src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs`:
- Around line 142-148: Remove the duplicate navigation method by consolidating
NavigateWry and NavigateToUrlWry into a single implementation: pick one method
name to keep (e.g., NavigateWry) and remove the other, then update all call
sites to use the retained method; ensure the retained method still guards on
_wryWebview != IntPtr.Zero and calls WryInterop.WebviewNavigate(_wryWebview,
url) so behavior is unchanged.
In `@src/wry-ffi/src/dialogs.rs`:
- Around line 212-219: The function wry_dialog_prompt_result_free currently
calls CString::from_raw without a panic guard, which can unwind across the FFI
boundary if the pointer is corrupted; wrap the unsafe CString::from_raw and drop
call inside std::panic::catch_unwind (same pattern used in
wry_dialog_selection_free) so any panic is contained, check
result.value.is_null() before calling, and silently ignore or log a panic
outcome instead of letting it unwind across FFI.
- Around line 87-102: wry_dialog_selection_free currently frees selection.paths
and calls CString::from_raw inside the function body without a panic guard,
risking unwinding across the FFI boundary; wrap the entire unsafe block (the
logic that reads selection.count, selection.paths, constructs the slice/Box and
calls CString::from_raw) in the existing panic guard (e.g., call
guard_panic_bool or the unit-returning guard used by other FFI functions) so any
panic is caught before crossing the FFI boundary, keeping the same behavior for
selection.count == 0 or selection.paths.is_null().
In `@src/wry-ffi/src/tray.rs`:
- Around line 68-74: The listed macOS tray FFI functions (wry_tray_free,
wry_tray_set_title, wry_tray_set_tooltip, wry_tray_set_visible,
wry_tray_set_show_menu_on_left_click, wry_tray_set_menu) must be wrapped in
guard_panic to prevent Rust panics from unwinding across the FFI boundary;
locate each function and wrap its implementation body in guard_panic(|| { ... })
so any panic is caught and converted rather than unwinding, keeping the existing
unsafe/Box::from_raw or tray handle manipulation inside that closure and
returning the same outward ABI behavior.
In `@src/wry-ffi/src/webview.rs`:
- Around line 30-331: wry_webview_build currently can panic across the FFI
boundary; wrap the outer body of wry_webview_build (the logic inside
with_window(...) closure and its return) in a panic guard using
std::panic::catch_unwind (with AssertUnwindSafe) and on Err return
ptr::null_mut(); ensure the wrapped code still returns the same Option<*mut
WryWebviewHandle> on Ok so the existing flatten().unwrap_or(ptr::null_mut())
behavior remains unchanged, and keep inner catch_unwind for protocol handlers
as-is; target symbols to change: wry_webview_build, the closure passed to
with_window, and use AssertUnwindSafe around that closure to prevent unwinding
into FFI.
🟡 Minor comments (12)
src/wry-ffi/src/shortcuts.rs-82-96 (1)
82-96:⚠️ Potential issue | 🟡 Minor
NEXT_IDcan wrap around to 0, which is the error sentinel.
fetch_addonAtomicU32wraps atu32::MAX. If it wraps to 0, a successful registration would return 0, which callers interpret as failure. Practically this requires ~4 billion registrations, so it's extremely unlikely, but a simple guard would make it correct:Suggested fix
- let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + let mut id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + if id == 0 { + id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + }src/wry-ffi/Cargo.toml-62-64 (1)
62-64:⚠️ Potential issue | 🟡 Minor
devtoolsfeature flag is redundant — devtools is already always enabled.Line 13 includes
"devtools"in the default wry features for all builds. Thedevtoolsfeature on line 64 re-enables the same wry feature, so toggling it has no effect. Either remove"devtools"from line 13's default features (to make it opt-in) or remove the feature flag.src/TauriCSharp/TauriCSharp/TauriApp.cs-177-204 (1)
177-204:⚠️ Potential issue | 🟡 MinorInconsistent window lifecycle:
TryRouteEventdisposes the window,DispatchAppEventdoes not.In
DispatchAppEvent(line 187), a destroyed/close-requested window is unregistered but not disposed. InTryRouteEvent(line 235), the window is both unregistered and disposed. This means windows created viaTauriApp.Run()rely on theDispose()call in lines 294-298, while windows created via the single-window path get eagerly disposed. This inconsistency could lead to native resource leaks ifTauriApp.Dispose()is never called afterRun()exits.src/TauriCSharp/TauriCSharp/GlobalShortcuts.cs-42-46 (1)
42-46:⚠️ Potential issue | 🟡 Minor
Unregisterremoves the callback before confirming native unregister succeeded.If
WryInterop.ShortcutUnregisterfails, the shortcut remains active at the OS level but the callback is already discarded, causing triggered events to be silently dropped.♻️ Suggested fix
public static bool Unregister(uint shortcutId) { - _callbacks.TryRemove(shortcutId, out _); - return WryInterop.ShortcutUnregister(shortcutId); + var result = WryInterop.ShortcutUnregister(shortcutId); + if (result) + { + _callbacks.TryRemove(shortcutId, out _); + } + return result; }src/wry-ffi/src/events.rs-143-147 (1)
143-147:⚠️ Potential issue | 🟡 MinorInconsistent JSON key casing:
isFocusedvs. snake_case everywhere else.All other event payload keys use
snake_case(window_id,scale_factor,device_id, etc.), butFocuseduses camelCaseisFocused. This will be a stumbling block for consumers expecting a uniform naming convention.♻️ Suggested fix
TaoWindowEvent::Focused(focused) => json!({ "type": "window-focused", "window_id": format!("{window_id:?}"), - "isFocused": focused, + "is_focused": focused, }),src/TauriCSharp/TauriCSharp/Dialogs.cs-40-43 (1)
40-43:⚠️ Potential issue | 🟡 Minor
DialogSelectionFreeis not in afinallyblock — native selection leaks ifExtractPathsthrows.If
ExtractPathsthrows (e.g., corrupted native pointer), theWryInterop.DialogSelectionFree(selection)call on line 42 is skipped. The same pattern applies toSaveFile(line 81). Wrap the extraction and free in a try/finally.♻️ Suggested fix (OpenFile)
- var selection = WryInterop.DialogOpen(in options); - var paths = ExtractPaths(selection); - WryInterop.DialogSelectionFree(selection); - return paths; + var selection = WryInterop.DialogOpen(in options); + try + { + return ExtractPaths(selection); + } + finally + { + WryInterop.DialogSelectionFree(selection); + }src/wry-ffi/src/dialogs.rs-133-136 (1)
133-136:⚠️ Potential issue | 🟡 Minor
YesNoCancelsilently falls through toYesNo, dropping the Cancel option.
tinyfiledialogsdoesn't expose a three-button Yes/No/Cancel variant, so this is an inherent limitation, but the caller has no way to distinguish "No" from "Cancel." Consider either:
- Documenting this limitation in a comment, or
- Removing
YesNoCancelfrom the enum if it can't be properly supported.src/wry-ffi/src/helpers.rs-283-295 (1)
283-295:⚠️ Potential issue | 🟡 Minor
write_positionandwrite_sizedereferencetargetwithout null-checking.If a caller passes a null
WryPoint*orWrySize*, these will segfault. Add a null guard or document the precondition.Proposed fix
pub fn write_position(target: *mut WryPoint, position: LogicalPosition<f64>) { + if target.is_null() { return; } unsafe { (*target).x = position.x; (*target).y = position.y; } } pub fn write_size(target: *mut WrySize, size: LogicalSize<f64>) { + if target.is_null() { return; } unsafe { (*target).width = size.width; (*target).height = size.height; } }src/wry-ffi/README.md-60-69 (1)
60-69:⚠️ Potential issue | 🟡 MinorDocs: size/position parameter types don’t match FFI signatures.
The Rust FFI usesf64forwry_window_set_sizeandwry_window_set_position, but the README lists integer types. Please update the signatures to avoid interop confusion.src/wry-ffi/src/window.rs-785-843 (1)
785-843:⚠️ Potential issue | 🟡 MinorReturn false when icon creation fails. If
Icon::from_rgbafails, the current code converts the error toNonevia.ok(), which is then passed toset_window_icon(None). This clears any existing icon and returnstrue, hiding the error from the caller. The functions should returnfalseon icon creation failure instead of silently clearing the icon.🔧 Safer handling
pub extern "C" fn wry_window_set_icon_rgba( @@ ) -> bool { if rgba_data.is_null() || rgba_len == 0 { return false; } @@ - guard_panic_bool(|| { - let icon = Icon::from_rgba(data.to_vec(), width, height) - .map_err(|e| log::error!("Failed to create icon from RGBA: {e}")) - .ok(); - - with_window(window, |w| { - w.set_window_icon(icon); - true - }) - .unwrap_or(false) - }) + guard_panic_bool(|| { + let icon = match Icon::from_rgba(data.to_vec(), width, height) { + Ok(icon) => icon, + Err(e) => { + log::error!("Failed to create icon from RGBA: {e}"); + return false; + } + }; + with_window(window, |w| { + w.set_window_icon(Some(icon)); + true + }) + .unwrap_or(false) + }) } @@ pub extern "C" fn wry_window_set_icon_file( @@ ) -> bool { @@ - let icon = Icon::from_rgba(rgba.into_raw(), w, h) - .map_err(|e| log::error!("Failed to create icon: {e}")) - .ok(); - - with_window(window, |win| { - win.set_window_icon(icon); - true - }) - .unwrap_or(false) + let icon = match Icon::from_rgba(rgba.into_raw(), w, h) { + Ok(icon) => icon, + Err(e) => { + log::error!("Failed to create icon: {e}"); + return false; + } + }; + + with_window(window, |win| { + win.set_window_icon(Some(icon)); + true + }) + .unwrap_or(false) }) }src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs-180-267 (1)
180-267:⚠️ Potential issue | 🟡 MinorFree the webview URL string after native build to avoid a memory leak.
Marshal.StringToCoTaskMemUTF8allocates unmanaged memory forconfig.Url, but it's never freed. Following the same pattern asFreeWindowConfig()(which frees the URL immediately afterWindowBuild()), the URL should be freed immediately afterWebviewBuild()since the native function copies it.Proposed fix
// Create webview _wryWebview = WryInterop.WebviewBuild(_wryWindow, in webviewConfig); + FreeWebviewConfig(ref webviewConfig); // Don't free webviewConfig protocol data yet - it needs to stay alive // The protocol state is stored in _protocolState @@ private static void FreeWindowConfig(ref WryWindowConfig config) { if (config.Title != IntPtr.Zero) { Marshal.FreeCoTaskMem(config.Title); config.Title = IntPtr.Zero; } } + + private static void FreeWebviewConfig(ref WryWebviewConfig config) + { + if (config.Url != IntPtr.Zero) + { + Marshal.FreeCoTaskMem(config.Url); + config.Url = IntPtr.Zero; + } + }src/TauriCSharp/TauriCSharp/TauriLog.cs-229-230 (1)
229-230:⚠️ Potential issue | 🟡 Minor
DevToolsControlNoteusesLogLevel.Informationbut is placed in the "Warning" section (EventId 100–199).This method is in the "Warning - Feature Limitations" block (EventId 100–199), yet its level is
Information. Either promote it toWarningfor consistency with its neighbors, or move it to a separate Information section with an appropriate EventId range.Proposed fix (if it should be Warning like the rest of the section)
- [LoggerMessage(EventId = 115, Level = LogLevel.Information, Message = "[{WindowTitle}] DevTools controlled via F12/context menu when devtools enabled at creation time")] + [LoggerMessage(EventId = 115, Level = LogLevel.Warning, Message = "[{WindowTitle}] DevTools controlled via F12/context menu when devtools enabled at creation time")]
🧹 Nitpick comments (24)
src/TauriCSharp/TauriCSharp/Ipc/TauriIpc.cs (1)
193-229:CancellationTokenSourceinPendingRequestis never disposed.
_ctsis created with a timeout (Line 205) which allocates an internal timer. NeitherCompletenorCanceldisposes it, andPendingRequestitself doesn't implementIDisposable. Over many requests this leaks timer handles until GC collects them.Pre-existing, but consider disposing
_ctsafter the TCS reaches a terminal state.♻️ Suggested fix — dispose CTS on completion
public void Complete(TauriMessage response) { if (response.Error != null) { _tcs.TrySetException(new TauriIpcException(response.Error, _correlationId)); } else { _tcs.TrySetResult(response); } + _cts.Dispose(); } public void Cancel() { _cts.Cancel(); _tcs.TrySetCanceled(); + _cts.Dispose(); }src/TauriCSharp/TauriCSharp/TauriNetDelegates.cs (2)
364-365: Stale TODO: the wry-ffi switch has already happened.Line 365 says
// TODO: Remove this limit when switching to wry-ffi, but lines 377–382 already describe the wry-ffi backend's constraint. This TODO should be resolved — either remove the 16-scheme limit if wry-ffi no longer imposes it, or update the comment to reflect the current state.
326-326: Simplify directforeachcast instead of using LINQ.Cast<>().
GetInvocationList()returnsDelegate[]. Theforeachstatement already performs the implicit cast toNavigationDelegate, making.Cast<NavigationDelegate>()redundant:Suggested simplification
- foreach (NavigationDelegate handler in handlers.GetInvocationList().Cast<NavigationDelegate>()) + foreach (NavigationDelegate handler in handlers.GetInvocationList())CLAUDE.md (1)
30-40: Add a language identifier to the fenced code block.The ASCII architecture diagram uses a fenced code block without a language specifier. Use
```textto satisfy markdownlint MD040 and signal that it's not executable code. The same applies to the inner diagram at Line 30 in theCLAUDE.mdcontext.As per static analysis hints, markdownlint flags: "Fenced code blocks should have a language specified (MD040)".
scripts/build-wry-ffi.sh (1)
8-8: Hardcoded .NET output path may not match actual build output.
DOTNET_BINis fixed toDebug/net8.0regardless of the--debug/release flag or .NET TFM. If a developer runsdotnet build -c Releaseor targetsnet9.0, the native library won't land in the right directory. Consider deriving the configuration and TFM, or at minimum accepting them as arguments.Suggested improvement
-DOTNET_BIN="$PROJECT_ROOT/src/TauriCSharp/TauriCSharp.TestApp/bin/Debug/net8.0" +DOTNET_CONFIG="Debug" +DOTNET_TFM="net8.0" # Parse arguments BUILD_TYPE="release" if [[ "$1" == "--debug" ]]; then BUILD_TYPE="debug" fi +if [[ -n "$2" ]]; then + DOTNET_CONFIG="$2" +fi +if [[ -n "$3" ]]; then + DOTNET_TFM="$3" +fi + +DOTNET_BIN="$PROJECT_ROOT/src/TauriCSharp/TauriCSharp.TestApp/bin/$DOTNET_CONFIG/$DOTNET_TFM"src/wry-ffi/src/shortcuts.rs (1)
148-161: Linear scan inpoll_hotkey_eventis O(n) per event.For the typical number of global shortcuts (< 20), this is fine. If the shortcut count grows significantly, consider a reverse map from
HotKey::id()→ local ID. Not urgent.src/TauriCSharp/TauriCSharp.TestApp/Program.cs (4)
166-173: Watchdog callsEnvironment.Exitfrom a background thread without cleanup.The
Task.Runwatchdog will abruptly terminate the process, bypassingDisposecalls and potentially leaving native resources (wry event loop, windows) un-freed. For a test harness this is acceptable, but consider logging to stdout as well (not just stderr) since autorun consumers may only capture stdout.
550-564: Roundabout newline handling with\\nliteral thenReplace.Lines 552-556 embed literal
\n(two characters) into theStringBuilder, then line 564 replaces them with real newlines. This is confusing and could be simplified by using actual newlines in the interpolated strings.♻️ Suggested simplification
- sb.Append($"Found {monitors.Count} monitor(s):\\n"); + sb.AppendLine($"Found {monitors.Count} monitor(s):"); foreach (var m in monitors) { - sb.Append($" {m.Name}: {m.MonitorArea.Width}x{m.MonitorArea.Height} @ ({m.MonitorArea.X},{m.MonitorArea.Y}), scale={m.Scale}\\n"); + sb.AppendLine($" {m.Name}: {m.MonitorArea.Width}x{m.MonitorArea.Height} @ ({m.MonitorArea.X},{m.MonitorArea.Y}), scale={m.Scale}"); } var current = _window.CurrentMonitor; if (current.HasValue) - sb.Append($"Current: {current.Value.Name}\\n"); + sb.AppendLine($"Current: {current.Value.Name}"); - sb.Append($"DPI: {_window.ScreenDpi}"); + sb.Append($"DPI: {_window.ScreenDpi}"); - Log(sb.ToString().Replace("\\n", "\n")); + Log(sb.ToString());
194-348:message.ToLower()in switch creates a lowered copy on every message but theStartsWithpatterns already embed lowercase prefixes.
message.ToLower()allocates a new string on every call. Since thecase var m when m.StartsWith(...)patterns use lowercase prefixes, consider usingStringComparison.OrdinalIgnoreCasein theStartsWithchecks and exactcasecomparisons instead. This also avoids locale-sensitive lowering issues.
826-834: JS injection only escapes single quotes but not backslashes in the command string.Line 830 escapes
'but a command payload containing\would break the JS string literal. The fixedAutorunCommandsset doesn't currently contain backslashes, but this is fragile for future additions.♻️ Suggested fix
- var escapedCommand = command.Replace("'", "\\'"); + var escapedCommand = command.Replace("\\", "\\\\").Replace("'", "\\'");src/TauriCSharp/TauriCSharp/GlobalShortcuts.cs (1)
61-67:DispatchShortcutEventshould catch callback exceptions to prevent poisoning the event loop.An unhandled exception in a user-provided callback would propagate up to the event loop dispatcher, potentially crashing the application.
♻️ Suggested fix
internal static void DispatchShortcutEvent(uint shortcutId) { if (_callbacks.TryGetValue(shortcutId, out var callback)) { - callback(shortcutId); + try + { + callback(shortcutId); + } + catch (Exception) + { + // Swallow to protect event loop; consider logging + } } }src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs (1)
111-126:SendWebMessageWryescaping is incomplete — missing tab, null byte, and backtick.The current escaping handles
\,',\n,\rbut misses\t,\0, and — if the message could ever contain backticks — template literal injection. For a JS single-quoted string, the most critical missing escape is the null byte which would silently truncate the string in some JS engines.src/TauriCSharp/TauriCSharp/TauriApp.cs (1)
166-175: Global shortcut ID cast frominttouintmay wrap on error values.Line 171:
(uint)shortcutIdEl.GetInt32()— if the native side ever sends a negative ID (e.g.,-1as an error sentinel), this silently wraps touint.MaxValueand would dispatch to a nonexistent callback. Consider usingGetUInt32()or adding a bounds check.src/TauriCSharp/TauriCSharp/Handles/WryAppHandle.cs (1)
123-144: LegacyWryAppHandleparameterless constructor eagerly creates a native event loop.The parameterless constructor (line 127-131) calls
WryEventLoopHandle.Create(), which allocates a native event loop. If the marshalling infrastructure ever instantiates this class (SafeHandle subclasses require a parameterless constructor for P/Invoke out-parameter marshalling), it would create an unintended event loop. Consider deferring theWryEventLoopHandle.Create()call to the staticCreate()factory method and making the default constructor create an invalid wrapper.♻️ Suggested fix
[System.Obsolete("Use WryEventLoopHandle instead")] internal sealed class WryAppHandle : SafeHandle { - private readonly WryEventLoopHandle _eventLoop; + private WryEventLoopHandle? _eventLoop; public WryAppHandle() : base(IntPtr.Zero, ownsHandle: false) { - _eventLoop = WryEventLoopHandle.Create(); - SetHandle(_eventLoop.DangerousGetRawHandle()); } - public override bool IsInvalid => _eventLoop.IsInvalid; + public override bool IsInvalid => _eventLoop?.IsInvalid ?? true; - public static WryAppHandle Create() => new(); + public static WryAppHandle Create() + { + var h = new WryAppHandle(); + h._eventLoop = WryEventLoopHandle.Create(); + h.SetHandle(h._eventLoop.DangerousGetRawHandle()); + return h; + } - public IntPtr DangerousGetRawHandle() => _eventLoop.DangerousGetRawHandle(); + public IntPtr DangerousGetRawHandle() => _eventLoop?.DangerousGetRawHandle() ?? IntPtr.Zero; protected override bool ReleaseHandle() { - _eventLoop.Dispose(); + _eventLoop?.Dispose(); return true; } }src/wry-ffi/src/tray.rs (2)
49-49: Redundantset_show_menu_on_left_clickcall.Line 38 already passes
cfg.show_menu_on_left_clickto the builder viawith_menu_on_left_click. Line 49 sets it again on the built tray, which is a no-op duplication.Proposed fix
if !cfg.visible { let _ = tray.set_visible(false); } - tray.set_show_menu_on_left_click(cfg.show_menu_on_left_click); - let identifier = CString::new(tray.id().as_ref())
192-196: Double clone of menu — one clone is redundant.
menu_handle.menu.clone()producescloned_menu, thencloned_menu.clone()is passed toset_menuwhile the original is stored. Clone directly from the source twice, or restructure to avoid the extra copy.Proposed fix
- let cloned_menu = menu_handle.menu.clone(); - tray.tray.set_menu(Some( - Box::new(cloned_menu.clone()) as Box<dyn tray_icon::menu::ContextMenu> - )); - tray.menu = Some(cloned_menu); + tray.tray.set_menu(Some( + Box::new(menu_handle.menu.clone()) as Box<dyn tray_icon::menu::ContextMenu> + )); + tray.menu = Some(menu_handle.menu.clone());src/wry-ffi/src/webview.rs (1)
278-327: Linux GTK vbox build path is duplicated betweenis_childand non-is_childbranches.Lines 278–291 and 303–316 are identical. Extract a helper or unify the branch since the GTK path doesn't differentiate between child and non-child webviews.
Sketch
- if cfg.is_child { - let bounds = Rect { ... }; - builder = builder.with_bounds(bounds); - - #[cfg(target_os = "linux")] - { - match w.default_vbox() { - Some(vbox) => builder.build_gtk(vbox).ok().map(|webview| ...), - None => None, - } - } - #[cfg(not(target_os = "linux"))] - { - builder.build_as_child(w).ok().map(|webview| ...) - } - } else { - #[cfg(target_os = "linux")] - { - match w.default_vbox() { - Some(vbox) => builder.build_gtk(vbox).ok().map(|webview| ...), - None => None, - } - } - #[cfg(not(target_os = "linux"))] - { - builder.build(w).ok().map(|webview| ...) - } - } + if cfg.is_child { + let bounds = Rect { + position: LogicalPosition::new(cfg.x, cfg.y).into(), + size: LogicalSize::new(cfg.width, cfg.height).into(), + }; + builder = builder.with_bounds(bounds); + } + + let build_result = { + #[cfg(target_os = "linux")] + { + match w.default_vbox() { + Some(vbox) => builder.build_gtk(vbox).ok(), + None => None, + } + } + #[cfg(not(target_os = "linux"))] + { + if cfg.is_child { + builder.build_as_child(w).ok() + } else { + builder.build(w).ok() + } + } + }; + + build_result.map(|webview| Box::into_raw(Box::new(WryWebviewHandle { + identifier: CString::new(format!("{}", webview.id())).unwrap_or_default(), + webview, + })))src/wry-ffi/src/helpers.rs (2)
69-92: Thread-local buffer pattern is sound but fragile — pointer validity depends on caller discipline.
write_json_to_bufferandwrite_string_to_bufferreturn a*const c_charwhose backingCStringlives in aRefCell. The pointer remains valid only until the next call that overwrites the same thread-local buffer. This is a well-known FFI pattern, but consider adding a doc comment on each function stating the returned pointer is invalidated by the next call to the same buffer, to protect future maintainers.
98-104:guard_panicis gated behind#[cfg(target_os = "macos")]— consider making it available on all platforms.Currently only tray (macOS-only) uses it, but
wry_webview_buildand other FFI entry points on all platforms would benefit from a pointer-returning panic guard. Removing the cfg gate is a zero-cost change that enables safer FFI wrappers elsewhere.src/TauriCSharp/TauriCSharp/Interop/WryTypes.cs (2)
325-336: Label fields inWryMessageDialogOptionsare unused dead code.The C# and Rust struct definitions match perfectly—both include
OkLabel,CancelLabel,YesLabel, andNoLabelfields in the same order with compatible types. No struct layout mismatch exists. However, the Rustwry_dialog_messagefunction reads onlytitle,message,level, andbuttons; the label fields are never accessed. They're harmless but unnecessary padding that should be considered for removal in a future refactor.
159-186: Field order and sizing are correct, but consider using the typedWryIpcHandlerdelegate for the IPC callback field.The field order matches the Rust definition exactly, and with
DisableRuntimeMarshallingenabled, C#boolcorrectly aligns to 1 byte. However, theIpcHandlerfield is stored asIntPtrrather than using theWryIpcHandlerdelegate type that's already defined in the codebase. While this works functionally, using the typed delegate would improve clarity and self-documentation of the field's purpose.src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs (1)
125-141: Avoid partial singleton state if proxy creation fails.
If proxy creation throws,_eventLoopstays assigned and later calls will skip proxy creation. Consider assigning both only after successful creation.💡 Suggested tweak
- _eventLoop = WryEventLoopHandle.Create(); - _eventLoopProxy = WryEventLoopProxyHandle.Create(_eventLoop); - return _eventLoop; + var loop = WryEventLoopHandle.Create(); + var proxy = WryEventLoopProxyHandle.Create(loop); + _eventLoop = loop; + _eventLoopProxy = proxy; + return _eventLoop;src/TauriCSharp/TauriCSharp/Interop/WryInterop.cs (2)
154-156: InconsistentCallConvCdeclqualification throughout the file.Some declarations use the short form
typeof(CallConvCdecl)(e.g., lines 155, 252, 470, 481, 518, 595, 618, etc.) while others use the fully qualifiedtypeof(System.Runtime.CompilerServices.CallConvCdecl). Both resolve identically thanks to theusingon line 4, but the inconsistency suggests copy-paste from different phases.Pick one style and apply it consistently.
28-47: Missing ownership documentation on returnedIntPtrstrings.
LibraryName(),CrateVersion(), andWebViewVersion()all returnIntPtr(presumably C strings), but lack the "do not free" annotation thatWindowTitle(line 371) and the monitor functions (lines 449, 457, 463) have. Clarifying ownership prevents use-after-free or leak bugs as the API surface grows.
| { | ||
| private static readonly Lazy<TauriApp> _instance = new(() => new TauriApp()); | ||
|
|
||
| /// <summary> | ||
| /// Gets the singleton TauriApp instance. | ||
| /// </summary> | ||
| public static TauriApp Instance => _instance.Value; | ||
|
|
||
| private readonly ConcurrentDictionary<string, TauriWindow> _windows = new(); | ||
| private readonly ConcurrentBag<TauriWindow> _pendingWindows = new(); | ||
| private readonly ILogger? _logger; | ||
| private bool _disposed; | ||
| private bool _shouldQuit; | ||
|
|
||
| /// <summary> | ||
| /// Event loop callback - pinned for lifetime of event loop. | ||
| /// </summary> | ||
| private WryEventLoopCallback? _appEventLoopCallback; | ||
|
|
||
| /// <summary> | ||
| /// Creates a new TauriApp instance. | ||
| /// </summary> | ||
| /// <param name="logger">Optional logger for application-level logging.</param> | ||
| private TauriApp(ILogger? logger = null) | ||
| { | ||
| _logger = logger; | ||
| } |
There was a problem hiding this comment.
Singleton constructor ignores logger — application-level logging is always disabled.
The private constructor accepts an ILogger? parameter (line 52), but the Lazy<TauriApp> initializer on line 30 invokes new TauriApp() with no arguments, so _logger is always null. The LogError calls on lines 141 and 202 will never produce output.
Consider either accepting an ILogger via an Initialize(ILogger) method before first use, or removing the logger parameter from the constructor.
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/TauriApp.cs` around lines 29 - 55, The
singleton's Lazy<TauriApp> (_instance) always calls new TauriApp() so the
constructor's ILogger? parameter is never used and _logger remains null; add a
static Initialize(ILogger logger) method and make the instance logger writable
(remove readonly on _logger or add an internal SetLogger method) so callers can
provide an ILogger before first use (Initialize should call Instance and set
Instance._logger or call an internal SetLogger on the singleton), or
alternatively remove the logger parameter from the private TauriApp constructor
and the _logger field entirely if you choose to drop per-app logging; update
references to _logger accordingly (e.g., in LogError calls).
- Fix WebviewIdentifier doc to say "do not free" (matches actual ownership) - Normalize all CallConvCdecl references to fully qualified form - Add volatile to TauriApp._shouldQuit for cross-thread visibility - Expose TauriApp.Logger property so singleton can actually log - Fix mimePtr leak in protocol responses (pack body+mime ptrs for cleanup) - Remove dead NavigateToUrlWry method (duplicate of NavigateWry) - Add panic guards to wry_dialog_selection_free, wry_dialog_prompt_result_free - Add panic guards to all tray mutators (free, set_title, set_tooltip, etc.) - Add panic guard to wry_webview_build and wry_webview_free - Make guard_panic<T> available cross-platform (was macOS-only) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs (1)
673-710:⚠️ Potential issue | 🟡 MinorFinalizer calls unmanaged FFI functions which may crash during shutdown.
Dispose(false)from the finalizer callsWryInterop.WebviewFreeandWryInterop.WindowFree. During process/AppDomain shutdown, the native library may already be unloaded, causing an access violation. Typically, finalizers should avoid P/Invoke calls. TheDispose(true)path (explicit disposal) is the correct place for FFI cleanup.Consider guarding the unmanaged section or relying solely on explicit disposal:
🛡️ Proposed guard
- // Unmanaged cleanup - if (_wryWebview != IntPtr.Zero) - { - WryInterop.WebviewFree(_wryWebview); - _wryWebview = IntPtr.Zero; - } - - if (_wryWindow != IntPtr.Zero) - { - WryInterop.WindowFree(_wryWindow); - _wryWindow = IntPtr.Zero; - _nativeInstance = IntPtr.Zero; - } + // Unmanaged cleanup - only safe during explicit disposal, not from finalizer + if (disposing) + { + if (_wryWebview != IntPtr.Zero) + { + WryInterop.WebviewFree(_wryWebview); + _wryWebview = IntPtr.Zero; + } + + if (_wryWindow != IntPtr.Zero) + { + WryInterop.WindowFree(_wryWindow); + _wryWindow = IntPtr.Zero; + _nativeInstance = IntPtr.Zero; + } + }
🤖 Fix all issues with AI agents
In `@src/TauriCSharp/TauriCSharp/TauriApp.cs`:
- Around line 171-180: The code in TauriApp.cs is unsafely casting
shortcutIdEl.GetInt32() to uint which will wrap negative values; modify the
global-shortcut handling so you read the int via GetInt32/TryGetInt32, validate
it's non-negative (>= 0) before casting to uint, and if invalid log or ignore
the event instead of dispatching GlobalShortcuts.DispatchShortcutEvent; apply
the same defensive check to the analogous cast in TauriWindow.Wry.cs (the code
that currently casts GetInt32() to uint) so negative JSON values are rejected or
handled safely.
- Around line 77-98: The Run() method registers _appEventLoopCallback with
callbackRegistry.Register and then calls WryInterop.EventLoopPump, but if
EventLoopPump throws the callback entry is never removed; wrap the EventLoopPump
call and the subsequent callbackRegistry.Unregister(IntPtr.Zero) in a
try/finally (register before try, call EventLoopPump in try, and always call
callbackRegistry.Unregister(IntPtr.Zero) in finally) to ensure the registry
entry is cleaned up even on exceptions; keep usage of
TauriWindow.EnsureEventLoopStatic() and TauriWindow.CallbackRegistryStatic
unchanged and ensure _appEventLoopCallback is still set before registering.
- Around line 182-195: In DispatchAppEvent, when routing window-specific events
(the block using _windows.TryGetValue and window.DispatchEventFromApp) ensure
the window is disposed after it is unregistered on "window-destroyed" or
"window-close-requested": mirror the behavior in TryRouteEvent by calling
UnregisterWindow(windowId) followed by window.Dispose() when
window.ShouldExitFromApp is true so native/webview handles and IPC/protocol
state are released promptly; locate the DispatchAppEvent method and the
window/ShouldExitFromApp, UnregisterWindow, and TryRouteEvent references to
implement this cleanup.
In `@src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs`:
- Around line 456-466: RunEventLoop registers _eventLoopCallback with
_callbackRegistry but never unregisters it, leaving a stale entry; wrap the call
to WryInterop.EventLoopPump(...) in a try/finally and in the finally block call
_callbackRegistry.Unregister(_wryWindow) and clear _eventLoopCallback (set to
null) so the callback is removed and the reference released when the pump
returns or an exception occurs.
- Around line 243-267: The BuildWebviewConfig method allocates unmanaged memory
for config.Url via Marshal.StringToCoTaskMemUTF8 but never frees it; update
CreateWryWindow (where WebviewBuild is called) to free that allocation after the
native call succeeds or fails (similar to how FreeWindowConfig frees
config.Title): capture the config returned from BuildWebviewConfig, call
WebviewBuild, then call Marshal.FreeCoTaskMem(config.Url) (or equivalent
cleanup) in a finally block so config.Url is always freed; ensure the same
pattern used for FreeWindowConfig is followed to avoid leaks.
In `@src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs`:
- Around line 111-126: Replace the ad-hoc escaping in SendWebMessageWry with
JSON string serialization: instead of manually replacing backslashes, single
quotes, newlines, etc., call System.Text.Json.JsonSerializer.Serialize(message)
to produce a safe JS string literal, then use that serialized value in the
script passed to WryInterop.WebviewEvaluateScript (e.g., call
window.ipc.receive(...) with the serialized string). This ensures correct
escaping of NUL, tabs, unicode line separators (\u2028/\u2029) and other edge
cases and avoids script injection or broken JavaScript.
- Around line 329-365: The four dead stubs GetResizableNotSupported,
SetResizableNotSupported, GetTopmostNotSupported, and SetTopmostNotSupported
should be removed or implemented: either delete these unused private methods
(and any internal references) to eliminate vestigial code, or implement them to
call the existing interop helpers (e.g., use WryInterop.WindowSetResizable and
WryInterop.WindowSetAlwaysOnTop for setters and, if available,
WryInterop.WindowGetResizable/WryInterop.WindowGetAlwaysOnTop for getters or
fall back to reading/writing _startupParameters) so the runtime behavior matches
the public API property semantics; update any callers to use the implemented
methods or the interop calls directly.
In `@src/wry-ffi/src/dialogs.rs`:
- Around line 136-139: The YesNoCancel variant of WryMessageDialogButtons
currently silently downgrades to a two-button dialog via
tinyfiledialogs::message_box_yes_no, which cannot represent a Cancel result;
update the match arm handling WryMessageDialogButtons::YesNoCancel in dialogs.rs
to explicitly perform the same tinyfiledialogs::message_box_yes_no call but add
a clear comment that YesNoCancel falls back to YesNo due to tinyfiledialogs
limitations, and update the WryMessageDialogButtons enum documentation to state
this FFI-layer fallback so callers are not misled about three-button semantics
(symbols: WryMessageDialogButtons, message_box_yes_no).
In `@src/wry-ffi/src/helpers.rs`:
- Around line 282-294: write_position and write_size currently dereference the
incoming raw pointers (*target) without null checks which can cause UB if
callers pass null; update both functions (write_position and write_size) to
check if target.is_null() and return early when null, otherwise perform the
unsafe write block as before, mirroring the null-guard pattern used by other
helpers in this module so WryPoint/WrySize fields are only assigned when target
is non-null.
🧹 Nitpick comments (9)
src/wry-ffi/src/tray.rs (2)
38-49: Redundantset_show_menu_on_left_clickcall after builder already applies it.Line 38 passes
show_menu_on_left_clickto the builder, and line 49 sets the same value again on the already-built tray. One of these is unnecessary.Suggested fix
- builder = builder.with_menu_on_left_click(cfg.show_menu_on_left_click); let tray = match builder.build() { Ok(tray) => tray, Err(_) => return ptr::null_mut(), }; if !cfg.visible { let _ = tray.set_visible(false); } - - tray.set_show_menu_on_left_click(cfg.show_menu_on_left_click);
83-90:wry_tray_identifieris the only FFI export without a panic guard.While
as_ref()andas_ptr()are unlikely to panic in practice, this is the sole exported function in the file not wrapped in a panic catch. For consistency and defense against UB from unexpected panics (e.g., corrupted handle), consider wrapping it:Suggested fix
#[cfg(target_os = "macos")] #[no_mangle] pub extern "C" fn wry_tray_identifier(tray: *mut WryTrayHandle) -> *const c_char { - let Some(tray) = (unsafe { tray.as_ref() }) else { - return ptr::null(); - }; - tray.identifier.as_ptr() + guard_panic(|| { + let Some(tray) = (unsafe { tray.as_ref() }) else { + return ptr::null_mut(); + }; + tray.identifier.as_ptr() as *mut c_char + }) as *const c_char }src/wry-ffi/src/helpers.rs (1)
68-92: Thread-local buffer pointer is valid only until the next write to the same buffer.The returned
*const c_charfromwrite_json_to_buffer/write_string_to_bufferis invalidated by the next call that writes to the same thread-local key (theRefCell's innerCStringgets replaced, deallocating the old buffer). This is a common FFI scratch-buffer pattern, but the contract is implicit.Consider adding a
# Safetydoc comment to both functions clarifying that the caller must copy or consume the pointer before the next call that writes to the same buffer key.src/wry-ffi/src/webview.rs (2)
358-469: Navigation, script, zoom, visibility, bounds, DevTools, and clear functions lack panic guards.These
extern "C"functions call intowry::WebViewmethods withoutcatch_unwind. While wry methods generally returnResultrather than panic, an unexpected panic here would unwind across the FFI boundary (UB). This is inconsistent withwry_webview_buildandwry_webview_freewhich do have guards, and with all dialog FFI functions indialogs.rs.Consider wrapping each in
guard_panic_boolfor consistency:Example for wry_webview_navigate
pub extern "C" fn wry_webview_navigate( webview: *mut WryWebviewHandle, url: *const c_char, ) -> bool { + guard_panic_bool(|| { let Some(url_str) = opt_cstring(url) else { return false; }; let Ok(parsed_url) = Url::parse(&url_str) else { return false; }; with_webview(webview, |view| view.load_url(parsed_url.as_str()).is_ok()).unwrap_or(false) + }) }The same pattern applies to
wry_webview_reload,wry_webview_evaluate_script,wry_webview_set_zoom,wry_webview_show,wry_webview_hide,wry_webview_set_bounds,wry_webview_open_devtools,wry_webview_close_devtools, andwry_webview_clear_browsing_data.
279-328: Linux child vs. non-child build paths are identical.Both the child (lines 282–291) and non-child (lines 307–316) Linux paths use
build_gtk(vbox)with the sameWryWebviewHandleconstruction. The only behavioral difference is that the child path setswith_boundsbeforehand (line 277). Consider extracting the Linux GTK build into a local closure to reduce duplication, though this is a minor concern given the code's clarity.src/TauriCSharp/TauriCSharp/Interop/WryInterop.cs (1)
694-702: Legacy shims may silently hide bugs.
StringFreeis a no-op andGetLastErroralways returnsIntPtr.Zero. If any caller relies onGetLastErrorto detect failures, errors will be silently swallowed. Consider adding a[Obsolete]attribute to both methods so callers get compiler warnings during the migration, rather than silently succeeding.♻️ Proposed change
- // String free - no longer needed in new API, but kept for compatibility - public static void StringFree(IntPtr _) + // String free - no longer needed in new API, but kept for compatibility + [System.Obsolete("StringFree is a no-op in the Velox-based API. Remove calls to this method.")] + public static void StringFree(IntPtr _) { // New API uses internal buffers that don't need freeing // This is a no-op for compatibility } - // Get last error - new API doesn't have this, return empty string - public static IntPtr GetLastError() => IntPtr.Zero; + // Get last error - new API doesn't have this, return empty string + [System.Obsolete("GetLastError is a no-op stub in the Velox-based API. Remove calls to this method.")] + public static IntPtr GetLastError() => IntPtr.Zero;src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs (1)
209-216:InvokeWryruns actions inline — thread-safety caveat.The comment correctly notes that this is a simplification. If
InvokeWryis called from a non-event-loop thread (e.g., from an IPC handler callback), theworkItemwill execute on the calling thread rather than being marshalled to the event loop thread. This could cause thread-safety issues with UI operations that must run on the main thread.Consider at minimum documenting this limitation in the public-facing
InvokeAPI or logging a warning when called off the event loop thread.src/TauriCSharp/TauriCSharp/TauriApp.cs (1)
28-60: SingletonTauriAppis not disposable-safe —Dispose()doesn't reset the singleton.After
Dispose()is called,TauriApp.Instancestill returns the disposed instance. Any subsequent access (e.g.,CreateWindow,Run) will operate on a disposed object without any guard. Consider addingObjectDisposedExceptionchecks in public methods.src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs (1)
308-356: Only the first custom scheme is wired — additional schemes are silently dropped.
BuildProtocolConfigtakes onlyCustomSchemes.Keys.First()(line 312). If multiple schemes are registered, the rest are silently ignored. The comment acknowledges this, but consider at minimum logging a warning whenCustomSchemes.Count > 1so callers aren't surprised at runtime.♻️ Proposed warning
private void BuildProtocolConfig(ref WryWebviewConfig config) { // For now, we only support a single protocol handler (the first one) // In the future we could support multiple + if (CustomSchemes.Count > 1 && _logger != null) + { + _logger.LogWarning("Only one custom protocol scheme is supported; {Count} registered. Using '{Scheme}'.", + CustomSchemes.Count, CustomSchemes.Keys.First()); + } var firstScheme = CustomSchemes.Keys.First();
C# fixes: - TauriApp.Run(): wrap EventLoopPump in try/finally for callback cleanup - TauriApp: use TryGetUInt32 for shortcut ID instead of unchecked cast - TauriApp: dispose window on destroy (was only unregistering) - TauriWindow: free config.Url after WebviewBuild (memory leak) - TauriWindow: wrap RunEventLoop pump in try/finally for callback cleanup - TauriWindow: move FFI cleanup inside disposing guard (finalizer safety) - TauriWindow: use JsonSerializer for JS string escaping (security) - TauriWindow: remove 30 dead NotSupported stubs and ThrowNotSupported - TauriNativeParameters: fix "naviage" typo, update stale TODO comment - TestApp: use HashSet for RecordTest dedup instead of Contains scan Rust fixes: - dialogs: document YesNoCancel→YesNo fallback (tinyfiledialogs limitation) - helpers: add null checks to write_position/write_size Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs (1)
686-724:⚠️ Potential issue | 🟡 MinorWaitForClose() does not dispose native resources — real resource leak in single-window mode.
The finalizer path (line 728) does not clean up native resources because
WebviewFree/WindowFreeare guarded byif (disposing). While this trade-off is justified (lines 707–708: native library may be unloaded during finalization),WaitForClose()does not callDispose(), and the test app demonstrates typical usage without explicit cleanup. This creates a resource leak in single-window mode. The caveat that "callers must ensureDispose()is called" is not enforced in current code patterns. Either add automatic cleanup afterWaitForClose()returns, or add explicit documentation requiring developers to callDispose()explicitly in single-window applications.
🤖 Fix all issues with AI agents
In `@src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs`:
- Around line 235-254: ParseMonitorsFromNative (and the analogous
ParseSingleMonitorFromNative) leak native memory by calling
WryInterop.WindowAvailableMonitors / WindowMonitorFromPoint and not freeing the
returned pointer; fix by wrapping the returned IntPtr in the existing
WryNativeString disposable (e.g., using var nativeStr = new
WryNativeString(jsonPtr)) and read the JSON via nativeStr.Value instead of
Marshal.PtrToStringUTF8, ensuring the using scope covers JsonDocument.Parse so
the native string is freed when done; do the same wherever WryInterop.* methods
return heap strings (or call WryInterop.StringFree() if not using
WryNativeString).
🧹 Nitpick comments (12)
src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs (2)
193-200:InvokeWryexecutes inline with no thread affinity check — potential thread-safety risk.If this is ever called from a non-UI thread, the
workItemwill execute on the caller's thread, bypassing the event loop. Native window APIs typically require main-thread affinity. The comment acknowledges this, but consider at minimum adding a debug assertion or logging a warning when called off the main thread, so misuse is caught early during development.
298-311:SetIconFileWry— no validation on file path.Minor: if
pathis null or points to a non-existent file, the behavior depends entirely on the Rust side. Consider a guard or at least a null check onpathbefore calling into native code.src/wry-ffi/src/helpers.rs (2)
98-117: Consider logging caught panics for debuggability.All three
guard_panic*variants silently discard the panic payload. During development and debugging, this can make root-cause analysis difficult when an FFI call silently returns a default/null. Aneprintln!or structured log of the panic info would be low-cost and high-value.Example for guard_panic
pub fn guard_panic<T>(f: impl FnOnce() -> *mut T) -> *mut T { match catch_unwind(AssertUnwindSafe(f)) { Ok(ptr) => ptr, - Err(_) => std::ptr::null_mut(), + Err(e) => { + eprintln!("[wry-ffi] panic caught at FFI boundary: {:?}", e); + std::ptr::null_mut() + } } }
68-92: Thread-local buffer pointer contract is sound but fragile — brief doc note would help.The returned
*const c_charis only valid until the same buffer is written to again. This is standard for FFI scratch buffers, but a# Safetydoc comment on both functions would make the contract explicit for future maintainers.src/wry-ffi/src/dialogs.rs (1)
121-125: Extract the repeatedWryMessageDialogLevel → MessageBoxIconmapping.The identical
match options.levelblock appears inwry_dialog_message,wry_dialog_confirm, andwry_dialog_ask. A small helper keeps this in one place.Suggested helper
+fn tinyfd_icon_from_level(level: WryMessageDialogLevel) -> tinyfiledialogs::MessageBoxIcon { + match level { + WryMessageDialogLevel::Info => tinyfiledialogs::MessageBoxIcon::Info, + WryMessageDialogLevel::Warning => tinyfiledialogs::MessageBoxIcon::Warning, + WryMessageDialogLevel::Error => tinyfiledialogs::MessageBoxIcon::Error, + } +}Then replace each inline match with
let icon = tinyfd_icon_from_level(options.level);.Also applies to: 155-159, 176-180
src/TauriCSharp/TauriCSharp.TestApp/Program.cs (2)
828-835: JS string escaping is fragile — only single quotes are handled.The
Replace("'", "\\'")on line 831 doesn't handle backslashes, newlines, or other JS-special characters. Currently safe becauseAutorunCommandscontains only simple ASCII, but this will silently break if a command ever contains\,\n, or unicode escapes.Consider using
System.Text.Json.JsonSerializer.Serialize(command)which produces a properly escaped JSON string (also a valid JS string literal), then embed it without the outer quotes or use it directly.♻️ Safer escaping
- // Escape single quotes in command for JS string literal - var escapedCommand = command.Replace("'", "\\'"); - - // Self-advancing chain: send the command, then after delay send autorun-next - _window.ExecuteScript( - $"sendMessage('{escapedCommand}'); setTimeout(() => sendMessage('autorun-next'), {delayMs});"); + // Use JSON serializer for proper JS string escaping + var escapedCommand = System.Text.Json.JsonSerializer.Serialize(command); + + // Self-advancing chain: send the command, then after delay send autorun-next + _window.ExecuteScript( + $"sendMessage({escapedCommand}); setTimeout(() => sendMessage('autorun-next'), {delayMs});");
167-173: Watchdog usesEnvironment.Exit(2)from a background thread — no resource cleanup.The watchdog will terminate the process without disposing the window or flushing test results. For a test app this is acceptable as a last resort, but the test summary (lines 122–128) will never print on timeout.
Consider logging the partial results before exiting:
♻️ Print partial results on timeout
_ = Task.Run(async () => { await Task.Delay(30_000); Console.Error.WriteLine("AUTORUN TIMEOUT: 30 seconds elapsed without completion."); + Console.Error.WriteLine($"Partial results — Passed: {_passCount} Failed: {_failCount} Skipped: {_skipCount}"); Environment.Exit(2); });src/TauriCSharp/TauriCSharp/TauriApp.cs (2)
266-270:Windowsproperty allocates a newDictionaryon every access.Each call to
Windowscopies the entireConcurrentDictionaryinto a newDictionary. If this is called in a loop or frequently (e.g., from event handlers), this could create allocation pressure. Consider caching or documenting it as a snapshot-per-call property.
230-249:TryRouteEventduplicates the destroy/dispose logic fromDispatchAppEvent.Lines 241–248 mirror lines 193–199. If the lifecycle conditions evolve, both sites must be updated in lockstep. Consider extracting a shared helper.
src/TauriCSharp/TauriCSharp/TauriWindow.Wry.cs (3)
56-66: Static_eventLoopCallbackfield is set by instance methods — potential overwrite in multi-window scenarios.
_eventLoopCallback(line 59) is static, butRunEventLoop(line 467) sets it from an instance method. In the single-windowWaitForClosepath this is fine, but ifTauriApp.Run()also sets its own callback (viaTauriApp._appEventLoopCallback), both paths coexist. The static field here would hold a stale reference from the lastRunEventLoopcall.This appears to be safe in practice since
TauriApp.Run()uses its own non-static_appEventLoopCallbackfield, andRunEventLoopis only used in single-window mode. But the static field could be confusing for future maintainers.
314-362: Only the first custom scheme is wired — subsequent schemes are silently ignored.
BuildProtocolConfigtakesCustomSchemes.Keys.First()(line 318) and creates a single definition. If multiple schemes are registered (e.g.,appandcustom), only the first one works. The comment on line 316–317 documents this, but there's no warning emitted to help developers diagnose why their second scheme doesn't work.♻️ Log a warning when schemes are dropped
private void BuildProtocolConfig(ref WryWebviewConfig config) { // For now, we only support a single protocol handler (the first one) // In the future we could support multiple + if (CustomSchemes.Count > 1 && _logger != null) + { + _logger.LogWarning("Only 1 custom scheme is supported; {Count} registered — extras ignored", + CustomSchemes.Count); + } + var firstScheme = CustomSchemes.Keys.First();
128-142: Double-checked locking on_eventLoopwithoutvolatile— potential stale read on ARM.The outer null check on line 130 reads
_eventLoopwithout a memory barrier. On weakly-ordered architectures (ARM), a thread could see a non-null reference to a partially-constructed object. On x86/x64 (strong memory model), this is safe in practice.If ARM support matters, consider making
_eventLoopvolatile or usingVolatile.Read/Lazy<T>.
| private IReadOnlyList<Monitor> ParseMonitorsFromNative() | ||
| { | ||
| // wry-ffi sets min/max size at creation time only | ||
| // Log a warning if called after creation | ||
| if (_nativeInstance != IntPtr.Zero) | ||
| var jsonPtr = WryInterop.WindowAvailableMonitors(_wryWindow); | ||
| if (jsonPtr == IntPtr.Zero) | ||
| return []; | ||
|
|
||
| var json = Marshal.PtrToStringUTF8(jsonPtr); | ||
| if (string.IsNullOrEmpty(json)) | ||
| return []; | ||
|
|
||
| using var doc = JsonDocument.Parse(json); | ||
| var monitors = new List<Monitor>(); | ||
|
|
||
| foreach (var element in doc.RootElement.EnumerateArray()) | ||
| { | ||
| Log("Warning: SetMinSize after window creation is not supported with wry-ffi backend"); | ||
| monitors.Add(ParseMonitorFromJson(element)); | ||
| } | ||
|
|
||
| return monitors; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for string-free functions in C#
rg -n 'FreeString|free_string|wry_free|StringFree|drop_string' --type=cs
echo "---SEPARATOR---"
# Search for WindowAvailableMonitors and related methods
rg -n 'WindowAvailableMonitors|ParseMonitor' --type=cs -B2 -A5Repository: trevwilson/tauri-csharp
Length of output: 4981
🏁 Script executed:
# Check WryNativeString to understand the cleanup pattern
cat -n src/TauriCSharp/TauriCSharp/Interop/WryNativeString.cs
echo "---SEPARATOR---"
# Check ParseSingleMonitorFromNative (around line 259-270)
sed -n '255,271p' src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.csRepository: trevwilson/tauri-csharp
Length of output: 3349
Native string pointers from FFI calls are never freed — memory leak in two methods.
Both ParseMonitorsFromNative and ParseSingleMonitorFromNative retrieve native strings from the FFI but fail to free them. WryInterop.WindowAvailableMonitors and WindowMonitorFromPoint return heap-allocated strings that must be freed via WryInterop.StringFree(). The codebase already defines a WryNativeString disposable wrapper for exactly this purpose (see Interop/WryNativeString.cs). Use it:
using var nativeStr = new WryNativeString(jsonPtr);
var json = nativeStr.Value;This pattern is documented in WryNativeString and ensures cleanup when the using block exits.
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/TauriWindow.WryCompat.cs` around lines 235 - 254,
ParseMonitorsFromNative (and the analogous ParseSingleMonitorFromNative) leak
native memory by calling WryInterop.WindowAvailableMonitors /
WindowMonitorFromPoint and not freeing the returned pointer; fix by wrapping the
returned IntPtr in the existing WryNativeString disposable (e.g., using var
nativeStr = new WryNativeString(jsonPtr)) and read the JSON via nativeStr.Value
instead of Marshal.PtrToStringUTF8, ensuring the using scope covers
JsonDocument.Parse so the native string is freed when done; do the same wherever
WryInterop.* methods return heap strings (or call WryInterop.StringFree() if not
using WryNativeString).
|
@claude collect all PR feedback that hasn't been successfully addressed |
|
Claude finished @trevwilson's task in 1m 24s —— View job Outstanding PR Feedback SummaryI've analyzed all CodeRabbit review feedback across three review rounds. Here's what remains unaddressed: 🔴 Critical Issues (Blocking)1. Merge Conflicts (30 files)
🟠 Major Issues (8 unresolved)C# Memory Leaks & Resource Management:
JS Injection & Escaping:
FFI Safety:
🟡 Minor Issues (16 identified, 1 critical for correctness)Thread Safety:
Dead Code:
API Issues:
Documentation & Clarity:
🧹 Nitpick Issues (24 identified)These are minor style/quality improvements but not correctness issues. I won't list all 24 in detail, but key ones include:
📋 Items Outside Diff Range (6)These are issues in code not changed by this PR but worth noting:
✅ What HAS Been AddressedThe following major issues were fixed in commits 80519d9 and 4f18e55:
🎯 Recommended Next StepsBefore merge:
Post-merge improvements:
|
- Wrap DialogSelectionFree in finally blocks to prevent native memory leaks if ExtractPaths throws (Dialogs.cs) - Fix GlobalShortcuts.Unregister to call native unregister before removing callback, preserving callback on native failure - Add Dispose() call after WaitForClose event loop exits to clean up native window/webview handles - Log panic payloads in all guard_panic* FFI boundary handlers for debuggability (helpers.rs) - Extract tinyfd_icon() helper to deduplicate dialog level mapping across three message dialog functions (dialogs.rs) - Fix stale/incomplete doc comments in TauriNativeParameters: fill in placeholder descriptions, remove duplicated "Default is" text - Use JsonSerializer.Serialize for JS string escaping in TestApp autorun Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TauriCSharp/TauriCSharp/TauriWindow.cs (1)
555-565:⚠️ Potential issue | 🟡 MinorUse
_wryWindowconsistently instead of_nativeInstancefor window operations.
_nativeInstanceand_wryWindoware the same pointer (assigned at TauriWindow.Wry.cs:205 for backward compatibility). However, line 562 uses_nativeInstancewhile the WryCompat compatibility layer uses_wryWindowexclusively for all window operations. For consistency with the established pattern throughout the codebase, use_wryWindowhere.
🤖 Fix all issues with AI agents
In `@src/TauriCSharp/TauriCSharp/Dialogs.cs`:
- Around line 244-252: The result from WryInterop.DialogPrompt(in options) must
be freed even if Marshal.PtrToStringUTF8 throws: wrap the PtrToStringUTF8 logic
in a try/finally so WryInterop.DialogPromptResultFree(result) is always called.
Specifically, keep the existing 'result' variable, move the call to
DialogPromptResultFree into a finally block and perform the
Accepted/PtrToStringUTF8 work inside the try; ensure the 'result' is in scope
for the finally (as in the other dialog methods like OpenFile/SaveFile).
In `@src/TauriCSharp/TauriCSharp/GlobalShortcuts.cs`:
- Around line 55-59: UnregisterAll currently clears the managed _callbacks
before calling WryInterop.ShortcutUnregisterAll(), which can leave native
shortcuts registered if the native call fails; change the order to call
WryInterop.ShortcutUnregisterAll() first and only clear _callbacks after that
(matching the Unregister flow), so perform WryInterop.ShortcutUnregisterAll()
and on success call _callbacks.Clear().
In `@src/TauriCSharp/TauriCSharp/TauriWindow.cs`:
- Around line 186-203: Remove the duplicate XML <summary> on the ScreenDpi
property: consolidate into a single well-formed summary for the ScreenDpi
property (which documents that it returns 96 when _nativeInstance is IntPtr.Zero
and otherwise computes DPI via WryInterop.WindowScaleFactor(_wryWindow)). Edit
the XML docs above the ScreenDpi getter to keep only one <summary> block and
ensure the description matches the implementation that checks _nativeInstance
and calls WryInterop.WindowScaleFactor(_wryWindow).
- Around line 864-869: The public Parent property returns the constructor-only
field _dotNetParent while SetParent and SetModal assign _parentWindow, causing
Parent to not reflect later changes; make Parent return the effective parent
(e.g. change the getter in Parent to return _parentWindow ?? _dotNetParent) or
else unify by setting _parentWindow in the constructor instead of _dotNetParent,
and update any XML docs/comments to clarify whether constructor parent vs
late-bound parent are distinct; adjust references to Parent, _dotNetParent,
_parentWindow, SetParent, SetModal, and the constructor accordingly.
- Around line 88-90: MacOsVersion currently assumes
RuntimeInformation.OSDescription.Split(' ')[1] exists and will throw if the
OSDescription format changes; update the MacOsVersion getter (and keep
IsMacOsPlatform check) to defensively parse the OSDescription: first verify
OSDescription is non-empty, split or regex-match only when there are enough
tokens, attempt Version.TryParse on the candidate token(s) and return null on
failure, and catch any unexpected exceptions to return null instead of allowing
an IndexOutOfRangeException to propagate; reference the MacOsVersion property,
IsMacOsPlatform and RuntimeInformation.OSDescription when applying this change.
🧹 Nitpick comments (4)
src/wry-ffi/src/helpers.rs (1)
68-92: Returned pointer validity depends on caller discipline — document the contract.Both
write_json_to_bufferandwrite_string_to_bufferreturn a*const c_charwhose backingCStringlives inside theRefCell. The pointer is valid only until the next mutable write to the same thread-local buffer. This is a standard FFI buffer-reuse pattern but is easy to misuse.Consider adding a
# Safetydoc comment to both functions noting that the returned pointer is invalidated by the next call that writes to the same buffer.src/TauriCSharp/TauriCSharp/GlobalShortcuts.cs (1)
64-70: Consider guarding against callback exceptions.If a callback throws, it will propagate up into the event loop dispatcher, potentially crashing the pump. A try/catch around the invocation would improve resilience.
Proposed fix
internal static void DispatchShortcutEvent(uint shortcutId) { if (_callbacks.TryGetValue(shortcutId, out var callback)) { - callback(shortcutId); + try + { + callback(shortcutId); + } + catch (Exception ex) + { + System.Diagnostics.Debug.WriteLine($"GlobalShortcut callback error for ID {shortcutId}: {ex}"); + } } }src/TauriCSharp/TauriCSharp.TestApp/Program.cs (2)
12-17:_recordedTests(HashSet) is not thread-safe; event handlers fire from multiple threads.
RecordTestis called from event handlers (e.g.,OnSizeChanged,OnLocationChanged) that may fire on different threads, yetHashSet<string>is not thread-safe. Similarly,_passCount/_failCount/_skipCountare incremented without synchronization. For a test harness this is unlikely to cause visible issues, but it could produce flaky results under contention.Consider using
ConcurrentDictionary<string, bool>orHashSetwith a lock (as done elsewhere withConcurrentDictionaryinGlobalShortcuts).
168-173: WatchdogEnvironment.Exit(2)will bypass cleanup andWaitForCloseteardown.
Environment.Exitfrom a background thread will skip thefinallyblock inWaitForClose(includingDispose()), potentially leaking native resources. For a test app watchdog this is acceptable — just noting the trade-off. If you ever need graceful shutdown, consider signaling the event loop to close instead.
| var result = WryInterop.DialogPrompt(in options); | ||
| string? value = null; | ||
|
|
||
| if (result.Accepted && result.Value != IntPtr.Zero) | ||
| { | ||
| value = Marshal.PtrToStringUTF8(result.Value); | ||
| } | ||
|
|
||
| WryInterop.DialogPromptResultFree(result); |
There was a problem hiding this comment.
DialogPromptResultFree is not in a finally — native memory leak if PtrToStringUTF8 throws.
If Marshal.PtrToStringUTF8 on line 249 throws (e.g., access violation from a corrupt pointer), DialogPromptResultFree on line 252 is skipped, leaking the native WryPromptDialogResult. This is inconsistent with the OpenFile/SaveFile methods, which correctly use a nested try/finally for the selection free.
Proposed fix: wrap in try/finally like the other dialog methods
var result = WryInterop.DialogPrompt(in options);
- string? value = null;
-
- if (result.Accepted && result.Value != IntPtr.Zero)
+ try
{
- value = Marshal.PtrToStringUTF8(result.Value);
- }
+ string? value = null;
+ if (result.Accepted && result.Value != IntPtr.Zero)
+ {
+ value = Marshal.PtrToStringUTF8(result.Value);
+ }
+ return value;
+ }
+ finally
+ {
+ WryInterop.DialogPromptResultFree(result);
+ }
- WryInterop.DialogPromptResultFree(result);
- return value;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = WryInterop.DialogPrompt(in options); | |
| string? value = null; | |
| if (result.Accepted && result.Value != IntPtr.Zero) | |
| { | |
| value = Marshal.PtrToStringUTF8(result.Value); | |
| } | |
| WryInterop.DialogPromptResultFree(result); | |
| var result = WryInterop.DialogPrompt(in options); | |
| try | |
| { | |
| string? value = null; | |
| if (result.Accepted && result.Value != IntPtr.Zero) | |
| { | |
| value = Marshal.PtrToStringUTF8(result.Value); | |
| } | |
| return value; | |
| } | |
| finally | |
| { | |
| WryInterop.DialogPromptResultFree(result); | |
| } |
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/Dialogs.cs` around lines 244 - 252, The result
from WryInterop.DialogPrompt(in options) must be freed even if
Marshal.PtrToStringUTF8 throws: wrap the PtrToStringUTF8 logic in a try/finally
so WryInterop.DialogPromptResultFree(result) is always called. Specifically,
keep the existing 'result' variable, move the call to DialogPromptResultFree
into a finally block and perform the Accepted/PtrToStringUTF8 work inside the
try; ensure the 'result' is in scope for the finally (as in the other dialog
methods like OpenFile/SaveFile).
| public static bool UnregisterAll() | ||
| { | ||
| _callbacks.Clear(); | ||
| return WryInterop.ShortcutUnregisterAll(); | ||
| } |
There was a problem hiding this comment.
UnregisterAll clears callbacks before the native call — inconsistent with Unregister ordering.
Unregister (line 44) correctly calls the native side first, then removes from the dict. UnregisterAll does the opposite: it clears _callbacks first, then calls ShortcutUnregisterAll(). If the native call fails, the managed state is already wiped — the shortcuts remain registered natively but the callbacks are gone, leaving orphaned shortcuts with no way to clean them up.
Proposed fix: swap the order to match Unregister
public static bool UnregisterAll()
{
- _callbacks.Clear();
- return WryInterop.ShortcutUnregisterAll();
+ if (!WryInterop.ShortcutUnregisterAll())
+ return false;
+
+ _callbacks.Clear();
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static bool UnregisterAll() | |
| { | |
| _callbacks.Clear(); | |
| return WryInterop.ShortcutUnregisterAll(); | |
| } | |
| public static bool UnregisterAll() | |
| { | |
| if (!WryInterop.ShortcutUnregisterAll()) | |
| return false; | |
| _callbacks.Clear(); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/GlobalShortcuts.cs` around lines 55 - 59,
UnregisterAll currently clears the managed _callbacks before calling
WryInterop.ShortcutUnregisterAll(), which can leave native shortcuts registered
if the native call fails; change the order to call
WryInterop.ShortcutUnregisterAll() first and only clear _callbacks after that
(matching the Unregister flow), so perform WryInterop.ShortcutUnregisterAll()
and on success call _callbacks.Clear().
| /// Indicates the version of MacOS. Returns null on non-macOS platforms. | ||
| /// </summary> | ||
| public static Version MacOsVersion => IsMacOsPlatform ? Version.Parse(RuntimeInformation.OSDescription.Split(' ')[1]) : null; | ||
| public static Version? MacOsVersion => IsMacOsPlatform ? Version.Parse(RuntimeInformation.OSDescription.Split(' ')[1]) : null; |
There was a problem hiding this comment.
MacOsVersion parsing is fragile — will throw on unexpected OSDescription format.
RuntimeInformation.OSDescription.Split(' ')[1] assumes a specific format (e.g., "Darwin 23.1.0"). If the format differs or has fewer tokens, this throws IndexOutOfRangeException. Since this is a public static property, any accidental access could crash the app.
Proposed defensive fix
- public static Version? MacOsVersion => IsMacOsPlatform ? Version.Parse(RuntimeInformation.OSDescription.Split(' ')[1]) : null;
+ public static Version? MacOsVersion
+ {
+ get
+ {
+ if (!IsMacOsPlatform) return null;
+ var parts = RuntimeInformation.OSDescription.Split(' ');
+ return parts.Length > 1 && Version.TryParse(parts[1], out var v) ? v : null;
+ }
+ }🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/TauriWindow.cs` around lines 88 - 90,
MacOsVersion currently assumes RuntimeInformation.OSDescription.Split(' ')[1]
exists and will throw if the OSDescription format changes; update the
MacOsVersion getter (and keep IsMacOsPlatform check) to defensively parse the
OSDescription: first verify OSDescription is non-empty, split or regex-match
only when there are enough tokens, attempt Version.TryParse on the candidate
token(s) and return null on failure, and catch any unexpected exceptions to
return null instead of allowing an IndexOutOfRangeException to propagate;
reference the MacOsVersion property, IsMacOsPlatform and
RuntimeInformation.OSDescription when applying this change.
| /// </exception> | ||
| /// <summary> | ||
| /// Gets the DPI for the current monitor. Computes from the window's scale factor. | ||
| /// Returns 96 (standard DPI) if the window is not initialized. | ||
| /// </summary> | ||
| public uint ScreenDpi | ||
| { | ||
| get | ||
| { | ||
| // Screen DPI not yet supported in wry-ffi, return a reasonable default | ||
| return 96; // Standard DPI | ||
| if (_nativeInstance == IntPtr.Zero) | ||
| return 96; | ||
|
|
||
| if (WryInterop.WindowScaleFactor(_wryWindow, out var scaleFactor)) | ||
| return (uint)(96 * scaleFactor); | ||
|
|
||
| return 96; | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate <summary> XML doc block on ScreenDpi.
Lines 181–190 contain the original summary, and lines 187–190 add a second <summary> block. The compiler will use the last one, but the stacked summaries produce malformed XML docs. Remove the first <summary> block or consolidate.
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/TauriWindow.cs` around lines 186 - 203, Remove
the duplicate XML <summary> on the ScreenDpi property: consolidate into a single
well-formed summary for the ScreenDpi property (which documents that it returns
96 when _nativeInstance is IntPtr.Zero and otherwise computes DPI via
WryInterop.WindowScaleFactor(_wryWindow)). Edit the XML docs above the ScreenDpi
getter to keep only one <summary> block and ensure the description matches the
implementation that checks _nativeInstance and calls
WryInterop.WindowScaleFactor(_wryWindow).
| private readonly TauriWindow? _dotNetParent; | ||
| /// <summary> | ||
| /// Gets the reference to parent TauriWindow instance. | ||
| /// This property can only be set in the constructor and it is optional. | ||
| /// </summary> | ||
| public TauriWindow Parent { get { return _dotNetParent; } } | ||
| public TauriWindow? Parent { get { return _dotNetParent; } } |
There was a problem hiding this comment.
Parent property returns _dotNetParent (constructor-only), but SetParent/SetModal set _parentWindow — public API is inconsistent.
SetParent (line 2264) and SetModal (line 2278) assign to _parentWindow, but the Parent property (line 869) returns _dotNetParent, which is only set in the constructor. After calling SetParent(someWindow), window.Parent will still return null (or the constructor-supplied parent), which is misleading.
If these are intentionally separate concepts (constructor parent vs. late-bound parent), the naming and documentation should make that distinction clear. Otherwise, Parent should reflect the effective parent.
Possible fix: unify the parent fields or update the getter
public TauriWindow? Parent { get { return _parentWindow ?? _dotNetParent; } }Or if _dotNetParent is no longer needed:
- private readonly TauriWindow? _dotNetParent;
public TauriWindow? Parent { get { return _parentWindow; } }And set _parentWindow = parent in the constructor instead of _dotNetParent = parent.
Also applies to: 2255-2281
🤖 Prompt for AI Agents
In `@src/TauriCSharp/TauriCSharp/TauriWindow.cs` around lines 864 - 869, The
public Parent property returns the constructor-only field _dotNetParent while
SetParent and SetModal assign _parentWindow, causing Parent to not reflect later
changes; make Parent return the effective parent (e.g. change the getter in
Parent to return _parentWindow ?? _dotNetParent) or else unify by setting
_parentWindow in the constructor instead of _dotNetParent, and update any XML
docs/comments to clarify whether constructor parent vs late-bound parent are
distinct; adjust references to Parent, _dotNetParent, _parentWindow, SetParent,
SetModal, and the constructor accordingly.
1 participant
Summary
--autorunflag that self-drives 19 tests through the real IPC path with exit codes for CIArchitecture
Pump-based event loop with JSON-serialized events. Multi-window routing via window_id. Shadow state for GTK async property staleness.
Test plan
dotnet run --project src/TauriCSharp/TauriCSharp.TestApp/ -- --autorun— 32 pass, 0 fail, 5 skipped (exit 0)xvfb-run dotnet run --project ... -- --autorun🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores