Skip to content

fix: non-macOS notification focus detection#71

Merged
terisuke merged 1 commit intodevfrom
fix/notification-non-macos
Apr 5, 2026
Merged

fix: non-macOS notification focus detection#71
terisuke merged 1 commit intodevfrom
fix/notification-non-macos

Conversation

@terisuke
Copy link
Copy Markdown

@terisuke terisuke commented Apr 5, 2026

What

Linux でのターミナルフォーカス検出を追加、Windows は通知許可、未知の OS は通知抑制。

Why

terminalIsFocused() が macOS 以外で常に false を返し通知が常に発火していた。

Closes #68

Changes

  • notification/index.ts: Linux xdotool 対応、Windows false (通知許可)、unknown true (抑制)
  • test/notification.test.ts: 6テスト追加

Review

  • code-reviewer Agent: completed
  • Codex CLI: HIGH fixed — Windows return true→false に修正済み

Test plan

  • bun test test/notification — 8 pass
  • typecheck 13/13 pass

…r platforms

terminalIsFocused() now uses xdotool on Linux to detect terminal focus.
On Windows and other unsupported platforms, defaults to "focused" to
prevent spurious notifications.

Closes #68

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 5, 2026 10:47
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@terisuke terisuke merged commit 5180f20 into dev Apr 5, 2026
5 of 9 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes non-macOS notification behavior by changing how “terminal focus” is detected so notifications don’t fire while the user is actively in a terminal on supported platforms.

Changes:

  • Extend Notification.terminalIsFocused() to handle Linux/Windows/unknown platforms differently.
  • Add Linux xdotool-based active-window PID probing for focus detection.
  • Add unit tests covering win32/unsupported and several Linux xdotool output cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/opencode/src/notification/index.ts Updates cross-platform terminal focus detection logic used to suppress notifications while focused.
packages/opencode/test/notification.test.ts Adds tests for platform-specific terminalIsFocused() behavior and Linux xdotool parsing paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (result.code !== 0) return true
const pid = parseInt(result.text.trim(), 10)
if (isNaN(pid)) return true
return pid === process.pid || pid === process.ppid
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux focus detection compares the active window PID from xdotool getactivewindow getwindowpid to process.pid/process.ppid. That PID is the GUI window owner (e.g. gnome-terminal, alacritty, code), not the Node/Bun process running inside the terminal, so this will almost always return false even when the terminal is focused (and notifications will keep firing).

Consider mapping the returned PID to its executable/comm (e.g. read /proc/<pid>/comm or /proc/<pid>/exe, or ps -p <pid> -o comm=) and running it through the existing terminal() normalization, or alternatively use xdotool to query the active window class/name and compare via terminal().

Suggested change
return pid === process.pid || pid === process.ppid
const processResult = await Process.text(
["ps", "-p", String(pid), "-o", "comm="],
{ nothrow: true },
)
if (processResult.code !== 0) return true
const activeProcess = processResult.text.trim()
if (!activeProcess) return true
return terminal(activeProcess)

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +70
test("linux: returns true when xdotool returns matching PID", async () => {
;(Process as Record<string, unknown>).text = mock(async () => ({
code: 0,
stdout: Buffer.from(String(process.pid) + "\n"),
stderr: Buffer.alloc(0),
text: String(process.pid) + "\n",
}))
const result = await Notification.terminalIsFocused("linux")
expect(result).toBe(true)
})

test("linux: returns false when xdotool returns non-matching PID", async () => {
const fakePid = 99999
;(Process as Record<string, unknown>).text = mock(async () => ({
code: 0,
stdout: Buffer.from(String(fakePid) + "\n"),
stderr: Buffer.alloc(0),
text: String(fakePid) + "\n",
}))
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Linux tests assume xdotool ... getwindowpid returns process.pid, but in practice that command returns the PID of the active window owner (terminal emulator / editor), not the PID of the CLI process running inside the terminal. This makes the test pass while masking the likely real-world failure mode.

After updating Linux focus detection (e.g., resolve PID -> executable name and call terminal()), adjust the tests to stub realistic xdotool output and assert against that behavior.

Suggested change
test("linux: returns true when xdotool returns matching PID", async () => {
;(Process as Record<string, unknown>).text = mock(async () => ({
code: 0,
stdout: Buffer.from(String(process.pid) + "\n"),
stderr: Buffer.alloc(0),
text: String(process.pid) + "\n",
}))
const result = await Notification.terminalIsFocused("linux")
expect(result).toBe(true)
})
test("linux: returns false when xdotool returns non-matching PID", async () => {
const fakePid = 99999
;(Process as Record<string, unknown>).text = mock(async () => ({
code: 0,
stdout: Buffer.from(String(fakePid) + "\n"),
stderr: Buffer.alloc(0),
text: String(fakePid) + "\n",
}))
test("linux: returns true when xdotool returns a terminal window PID and it resolves to a known terminal app", async () => {
const activeWindowPid = 4242
;(Process as Record<string, unknown>).text = mock(async (...args: unknown[]) => {
const command = args.map(String).join(" ")
if (command.includes("xdotool") && command.includes("getwindowpid")) {
return {
code: 0,
stdout: Buffer.from(String(activeWindowPid) + "\n"),
stderr: Buffer.alloc(0),
text: String(activeWindowPid) + "\n",
}
}
return {
code: 0,
stdout: Buffer.from("Visual Studio Code\n"),
stderr: Buffer.alloc(0),
text: "Visual Studio Code\n",
}
})
const result = await Notification.terminalIsFocused("linux")
expect(result).toBe(true)
})
test("linux: returns false when xdotool returns a window-owner PID that resolves to a non-terminal app", async () => {
const activeWindowPid = 99999
;(Process as Record<string, unknown>).text = mock(async (...args: unknown[]) => {
const command = args.map(String).join(" ")
if (command.includes("xdotool") && command.includes("getwindowpid")) {
return {
code: 0,
stdout: Buffer.from(String(activeWindowPid) + "\n"),
stderr: Buffer.alloc(0),
text: String(activeWindowPid) + "\n",
}
}
return {
code: 0,
stdout: Buffer.from("Xcode\n"),
stderr: Buffer.alloc(0),
text: "Xcode\n",
}
})

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: non-macOS notification fires even when terminal is focused

2 participants