Repository navigation
feat: Rust tray runtime and universal macOS app for 2.0 - #16
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acb842b187
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Ok(executable) = std::env::current_exe() { | ||
| if std::process::Command::new(executable).spawn().is_ok() { | ||
| std::process::exit(0); |
There was a problem hiding this comment.
When /app/restart is used, the replacement process is spawned while the current process still owns ports 8000 and 8080; only after spawn() succeeds does the parent exit. The child can therefore reach start_servers first, fail both fixed-port binds, and terminate, leaving the application stopped—especially harmful for clean_start, which has already cleared the PIN. Use an external watcher that waits for the old PID to exit, or otherwise relinquish the listeners before launching the replacement.
Useful? React with 👍 / 👎.
| pub fn toggle_mute(state: &AppState) -> Result<bool, String> { | ||
| let next = !state.muted.load(std::sync::atomic::Ordering::Relaxed); | ||
| set_mute(state, next) |
There was a problem hiding this comment.
When the machine starts with audio already muted, or mute is changed outside PC Remote, this cached flag still begins at false and is never synchronized from the OS. On macOS the first attempt to unmute consequently sets mute to true again; on Windows the synthetic key may unmute the device while the API reports muted: true. Query the current endpoint mute state before toggling and before returning status.
Useful? React with 👍 / 👎.
| let actual = process.name().to_string_lossy(); | ||
| let expected = format!("{}.exe", search.name); | ||
| if actual.eq_ignore_ascii_case(&expected) | ||
| || actual.eq_ignore_ascii_case(&search.name) |
There was a problem hiding this comment.
For Windows entries discovered from .lnk files, search.name is the shortcut's display name rather than the process executable name—for example, Google Chrome.lnk produces Google Chrome, while the process is chrome.exe. The minimize and close buttons therefore report that no window was found for many normal applications. Resolve the shortcut/executable or retain a process identity when building the app item and match against that instead.
Useful? React with 👍 / 👎.
| pub fn local_ip() -> Ipv4Addr { | ||
| UdpSocket::bind((Ipv4Addr::UNSPECIFIED, 0)) | ||
| .and_then(|socket| { | ||
| socket.connect((Ipv4Addr::new(8, 8, 8, 8), 80))?; |
There was a problem hiding this comment.
On hosts with a VPN or multiple adapters, probing only the route to 8.8.8.8 can select an address that the phone cannot reach, even though migrated settings retain the user's network.preferred_ip and preferred_interface. That selected address is embedded in the QR URL and is also the only non-loopback address accepted by allowed_origin, so connecting through the intended LAN address is rejected. Restore preferred-interface handling and include all usable local addresses in the origin allowlist.
Useful? React with 👍 / 👎.
| let rows = entries | ||
| .iter() | ||
| .filter(|row| reset || since.is_none_or(|mark| row.id > mark)) | ||
| .take(limit.min(1000)) | ||
| .cloned() |
There was a problem hiding this comment.
When more than limit entries are buffered, this takes the oldest limit matching rows but returns the ID of the newest buffered entry as next_since. The web client requests 120 rows and adopts that cursor, so an initial fetch with 500 buffered entries displays the oldest 120 and permanently skips the remaining 380. Return the newest selected row's ID and select the latest limit rows when resetting or initially loading.
Useful? React with 👍 / 👎.
| let name = "System default audio output"; | ||
| #[cfg(target_os = "windows")] | ||
| let name = "System default audio output"; | ||
| json!({ "id": "default", "name": name }) |
There was a problem hiding this comment.
Both platform implementations return the same hard-coded default ID and display name without querying the operating system. Therefore /info and /audio/devices continue to show “System default audio output” regardless of whether the active device is speakers, headphones, HDMI, or another endpoint, so the advertised current-output status cannot reflect device changes. Query and return the active endpoint's real identifier and name.
Useful? React with 👍 / 👎.
| let script = match action { | ||
| "play_pause" => "tell application \"Music\" to playpause", | ||
| "next" => "tell application \"Music\" to next track", | ||
| "prev" => "tell application \"Music\" to previous track", | ||
| "stop" => "tell application \"Music\" to stop", |
There was a problem hiding this comment.
On macOS every media action is sent specifically to the Music application. When playback is coming from Spotify, a browser, VLC, a podcast app, or another common player, the controls affect Music instead of the currently playing media and may launch or manipulate Music unexpectedly. Emit system media-key events or use the active Now Playing session rather than hard-coding one application.
Useful? React with 👍 / 👎.
| for directory in [ | ||
| PathBuf::from("/Applications"), | ||
| PathBuf::from("/System/Applications"), | ||
| ] { |
There was a problem hiding this comment.
Applications installed only for the current user live under ~/Applications, but discovery checks only the two system-wide directories. Such apps never appear in the recent-app response and therefore cannot be launched or pinned from the controller. Include the user's Applications directory when collecting macOS candidates.
Useful? React with 👍 / 👎.
| pub fn autostart_path() -> Option<PathBuf> { | ||
| env::var_os("APPDATA") | ||
| .map(PathBuf::from) | ||
| .map(|path| path.join("Microsoft/Windows/Start Menu/Programs/Startup/PC Remote.cmd")) |
There was a problem hiding this comment.
Upgrading a user who still has the supported legacy PC-Android.cmd startup file leaves that entry active but invisible because the Rust runtime checks only PC Remote.cmd. The tray consequently reports autostart as disabled even though Windows still launches the app, and enabling it creates a second startup entry. Preserve the previous migration behavior by recognizing and replacing or removing the owned legacy entry.
Useful? React with 👍 / 👎.
| let state = Arc::new(state::AppState::new()); | ||
| let api_port = config::port("PC_REMOTE_API_PORT", config::API_PORT_DEFAULT); | ||
| let web_port = config::port("PC_REMOTE_WEB_PORT", config::WEB_PORT_DEFAULT); | ||
| if let Err(error) = api::start_servers(Arc::clone(&state), api_port, web_port) { |
There was a problem hiding this comment.
There is no process-level guard before the new runtime binds its fixed ports. When a user launches the installed shortcut while PC Remote is already running in the tray, the second process reports a fatal port-binding error instead of quietly exiting or activating the existing instance; duplicate startup entries can trigger the same modal error at every login. Acquire a named mutex before constructing the runtime, as the replaced Windows implementation did.
Useful? React with 👍 / 👎.
1 participant
PC Remote 2.0 replaces the Python desktop process with a Rust tray/menu-bar app that stays idle without periodic system polling, while keeping the existing phone browser controller and migrating legacy settings safely.
Validation completed locally: 12 Rust tests pass; Windows release build and Clippy pass; ARM64 and Intel macOS targets pass cross-target check and Clippy; live Windows API smoke verified pairing, PIN blocking, and session-token behavior. Real macOS app packaging and runtime checks are assigned to the macOS CI runners.
The macOS app is ad-hoc signed and is not notarized.
Closes #13