{{ message }}
fix(single-instance): improve Windows foreground activation#148
Closed
starSumi wants to merge 1 commit into
Closed
fix(single-instance): improve Windows foreground activation#148starSumi wants to merge 1 commit into
starSumi wants to merge 1 commit into
Conversation
starSumi
force-pushed
the
fix-windows-single-instance-focus
branch
from
8923f7c to
56638fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/single_instance.rs (1)
142-167: ⚡ Quick winAdd regression tests for the new IPC markers (
__PID__,__FOCUS__).The new protocol parsing path is currently untested. Please add unit/integration coverage for: valid PID + paths, PID-only focus activation, malformed
__PID__value, and ensuring marker lines are not treated as file paths.Also applies to: 291-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/single_instance.rs` around lines 142 - 167, Add unit tests for read_activation_from_stream covering the new IPC markers: create test cases that feed a TcpStream-like input containing lines with "__PID__:<value>", "__FOCUS__", and file paths and assert the returned InstanceActivation.paths and sender_pid; include tests for (1) valid PID plus multiple paths, (2) a focus-only activation where only "__FOCUS__" and a PID line are present, (3) a malformed "__PID__" value results in sender_pid == None, and (4) marker lines ("__PID__" and "__FOCUS__") are not included in paths. Locate tests around the read_activation_from_stream function and use the same InstanceActivation struct to assert expected behavior for each scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/file_ops.rs`:
- Around line 1494-1503: The current primary process is calling
super::platform::allow_foreground_from_process(activation.sender_pid) which
inverts the Windows API flow; instead persist or return the primary PID (via the
lock file or the TCP handshake) so the secondary process can call
AllowSetForegroundWindow(primary_pid) before it exits/forwards paths; remove the
sender_pid field and this call site from the primary (keep
ctx.send_viewport_cmd(...) focus/attention calls in the primary), and in the
secondary implementation ensure allow_foreground_from_process(primary_pid)
returns a bool and log an error when it returns false rather than discarding the
result.
In `@src/single_instance.rs`:
- Around line 192-196: The code currently writes the secondary's PID
("sender_pid") as "__PID__" which causes the primary to call
allow_foreground_from_process(activation.sender_pid) and incorrectly grant
AllowSetForegroundWindow to the secondary; fix by changing the protocol so the
PID sent in "__PID__" is the PID of the process that will actually call
SetForegroundWindow (the primary's PID) — e.g. replace sender_pid with a
target_pid (or rename to activation_target_pid) when building pid_line in
single_instance.rs and ensure the receiver code that calls
allow_foreground_from_process(activation.sender_pid) uses that PID (or rename
usage to activation.target_pid) so AllowSetForegroundWindow is called for the
process that will perform the activation.
---
Nitpick comments:
In `@src/single_instance.rs`:
- Around line 142-167: Add unit tests for read_activation_from_stream covering
the new IPC markers: create test cases that feed a TcpStream-like input
containing lines with "__PID__:<value>", "__FOCUS__", and file paths and assert
the returned InstanceActivation.paths and sender_pid; include tests for (1)
valid PID plus multiple paths, (2) a focus-only activation where only
"__FOCUS__" and a PID line are present, (3) a malformed "__PID__" value results
in sender_pid == None, and (4) marker lines ("__PID__" and "__FOCUS__") are not
included in paths. Locate tests around the read_activation_from_stream function
and use the same InstanceActivation struct to assert expected behavior for each
scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a74fde8f-37bc-471c-b714-6b67ea38402d
📒 Files selected for processing (3)
src/app/file_ops.rssrc/app/platform.rssrc/single_instance.rs
Comment on lines
+1494
to
+1503
| // On Windows, the secondary instance was launched directly by the user and | ||
| // owns foreground permission. Allow that PID to transfer foreground rights | ||
| // before we issue the normal eframe/egui focus request. | ||
| let _ = super::platform::allow_foreground_from_process(activation.sender_pid); | ||
|
|
||
| let time = self.get_app_time(); | ||
| let mut opened = 0; | ||
| // Keep the normal cross-platform focus path in place. | ||
| ctx.send_viewport_cmd(egui::ViewportCommand::Focus); | ||
| ctx.send_viewport_cmd(egui::ViewportCommand::RequestUserAttention( | ||
| egui::UserAttentionType::Informational, | ||
| )); |
There was a problem hiding this comment.
AllowSetForegroundWindow is called from the wrong process—foreground transfer will silently fail.
The Windows API requires the process that has foreground permission (the secondary instance launched by Explorer) to call AllowSetForegroundWindow(target_pid) to grant permission to the target. Here the primary (which lacks foreground permission) calls it with the secondary's PID—this inverts the intended flow.
Per issue #147's proposed solution: "call AllowSetForegroundWindow(primary_pid) from the secondary process before exit."
To fix this:
- Store the primary's PID in the lock file or send it back via TCP handshake
- Have the secondary call
AllowSetForegroundWindow(primary_pid)before forwarding paths - The
sender_pidfield and this call site can be removed from the primary
Additionally, discarding the result (let _ = ...) hides failures—at minimum, log when the call returns false to aid debugging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/file_ops.rs` around lines 1494 - 1503, The current primary process is
calling super::platform::allow_foreground_from_process(activation.sender_pid)
which inverts the Windows API flow; instead persist or return the primary PID
(via the lock file or the TCP handshake) so the secondary process can call
AllowSetForegroundWindow(primary_pid) before it exits/forwards paths; remove the
sender_pid field and this call site from the primary (keep
ctx.send_viewport_cmd(...) focus/attention calls in the primary), and in the
secondary implementation ensure allow_foreground_from_process(primary_pid)
returns a bool and log an error when it returns false rather than discarding the
result.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/technical/platform/single-instance.md`:
- Around line 54-60: The fenced startup-flow block in single-instance.md
triggers markdownlint MD040 because it lacks a language tag; update the
triple-backtick block that begins with "Parse CLI args → Read lock file → port
exists?" to include a language identifier (e.g., add "text" after the opening
```), so the block becomes ```text and satisfies the lint rule.
In `@src/single_instance.rs`:
- Around line 78-83: The lock file is being written before the pid file,
creating a race; change the order so write_pid_file(std::process::id()) is
attempted (and its error logged) before calling write_lock_file(port);
specifically update the sequence around the calls to write_pid_file and
write_lock_file in this block so instance.pid is created (or its failure logged)
prior to publishing instance.lock, keeping the existing warn! logs for each
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3e77515-370e-472b-9dae-89f591f82e3c
📒 Files selected for processing (4)
docs/technical/platform/single-instance.mdsrc/app/file_ops.rssrc/platform/mod.rssrc/single_instance.rs
Comment on lines
54
to
60
| ``` | ||
| Parse CLI args → Read lock file → port exists? | ||
| YES → connect to port (500ms timeout) | ||
| SUCCESS → send paths, shutdown(Write), exit | ||
| SUCCESS → (Windows: allow foreground for primary PID) → send paths, shutdown(Write), exit | ||
| FAIL → stale lock, delete, become primary | ||
| NO → become primary | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced startup-flow block.
markdownlint MD040 is triggered because the fenced block has no language tag.
Suggested fix
- ```
+ ```text
Parse CLI args → Read lock file → port exists?
YES → connect to port (500ms timeout)
SUCCESS → (Windows: allow foreground for primary PID) → send paths, shutdown(Write), exit
FAIL → stale lock, delete, become primary
NO → become primary</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/technical/platform/single-instance.md` around lines 54 - 60, The fenced
startup-flow block in single-instance.md triggers markdownlint MD040 because it
lacks a language tag; update the triple-backtick block that begins with "Parse
CLI args → Read lock file → port exists?" to include a language identifier
(e.g., add "text" after the opening ```), so the block becomes ```text and
satisfies the lint rule.
Source: Linters/SAST tools
Comment on lines
78
to
+83
| if let Err(e) = write_lock_file(port) { | ||
| warn!("Failed to write instance lock file: {}", e); | ||
| } | ||
| if let Err(e) = write_pid_file(std::process::id()) { | ||
| warn!("Failed to write instance pid file: {}", e); | ||
| } |
There was a problem hiding this comment.
Publish instance.lock only after instance.pid is written.
The lock file is the discovery signal for secondaries. Writing it before the pid file creates a small race where the secondary can connect but miss the foreground-permission handoff because instance.pid is not yet readable.
Suggested fix
- if let Err(e) = write_lock_file(port) {
- warn!("Failed to write instance lock file: {}", e);
- }
if let Err(e) = write_pid_file(std::process::id()) {
warn!("Failed to write instance pid file: {}", e);
}
+ if let Err(e) = write_lock_file(port) {
+ warn!("Failed to write instance lock file: {}", e);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/single_instance.rs` around lines 78 - 83, The lock file is being written
before the pid file, creating a race; change the order so
write_pid_file(std::process::id()) is attempted (and its error logged) before
calling write_lock_file(port); specifically update the sequence around the calls
to write_pid_file and write_lock_file in this block so instance.pid is created
(or its failure logged) prior to publishing instance.lock, keeping the existing
warn! logs for each failure.
OlaProeis
added a commit
that referenced
this pull request
Jun 9, 2026
Add contributor entry for PR #148. Tag the single-instance startup flow code fence with a language for markdownlint. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Ferrite already uses
ViewportCommand::Focuswhen paths arrive from the single-instance TCP listener, but on Windows that is not always enough when the window is already running in the background and the user double-clicks a file from Explorer.Change
ViewportCommand::Focus__PID__:<pid>from the secondary processAllowSetForegroundWindow(sender_pid)on Windows before the focus requestViewportCommand::RequestUserAttention(Informational)as a fallbackFixes #147
Validation
cargo check --message-format shortSummary by CodeRabbit
Improvements
Documentation