[codex] Support KDE Plasma Wayland#44
Open
bee-san wants to merge 2 commits intoksyasuda:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds KWin (KDE/Plasma Wayland) as a supported session/backend across type definitions, centralized session detection, CLI/config, launcher/mpv script options, a new KWin window-tracker (DBus + KWin script bridge), and accompanying TypeScript and Lua tests and scripts. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
bee-san
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/window-trackers/kwin-tracker.ts (1)
121-149: Takepsout of the KWin update hot path.With
targetMpvSocketPathset, every geometry/focus update can synchronously spawnpsonce per candidate window here. During drag/resize bursts that blocks the Electron main thread; caching PID-to-cmdline or reading/proc/<pid>/cmdlinedirectly would keep this path much cheaper.Also applies to: 434-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 121 - 149, The selectKWinMpvWindow hot path calls options.getWindowCommandLine for every candidate window (when targetMpvSocketPath is set), which currently synchronously spawns ps per PID and blocks the main thread; change this by introducing a short-lived PID->cmdline cache (or read /proc/<pid>/cmdline directly) and use that cache inside selectKWinMpvWindow and the other similar block (around lines 434-443) so you avoid spawning ps per window; update selectKWinMpvWindow to query the cache (e.g., getCachedCmdline(pid)) before falling back to an async-safe read, and ensure cache entries expire/refresh appropriately to keep behavior correct while removing synchronous ps calls from the KWin update hot path.launcher/mpv.ts (1)
228-250: Please share backend autodetection with the overlay side.This block now duplicates the KWin environment probing from
src/window-trackers/index.ts, but the two paths already diverge on sway detection (SWAYSOCKis only handled there). Keeping the session-env detection in one helper would avoid launcher-vs-tracker backend drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launcher/mpv.ts` around lines 228 - 250, The detectBackend function in launcher/mpv.ts duplicates KWin/Wayland probing that exists in src/window-trackers/index.ts, causing drift (notably SWAYSOCK handling only present in the tracker); extract the session-environment detection into a shared helper (e.g., export a new detectSessionBackend or move logic into an existing util) that centralizes checks for XDG_CURRENT_DESKTOP, XDG_SESSION_DESKTOP, XDG_SESSION_TYPE, WAYLAND_DISPLAY, HYPRLAND_INSTANCE_SIGNATURE, SWAYSOCK, and commandExists('hyprctl'), then have detectBackend call that shared helper and update src/window-trackers/index.ts to import and use it so both launcher (detectBackend) and the overlay/tracker use identical autodetection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/subminer/process.lua`:
- Around line 240-244: The backend allowlist in the option parsing branch (where
normalized_key == "backend") excludes "windows" so backend=windows overrides are
ignored; update the accepted backends list used when assigning local backend
(variable backend) so it includes "windows" alongside "auto", "hyprland",
"kwin", "sway", "x11", and "macos", and then set overrides.backend = backend as
before to ensure the override is honored (refer to normalized_key, backend local
variable, and overrides.backend to find and update the condition).
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 302-320: The temp script directory is created only in the
constructor so after stopAsync() removes it the instance becomes unusable on
restart; modify the lifecycle so the tempDir and scriptPath are (re)created
before any write operations (e.g., in start() / startAsync()) instead of only in
the constructor: remove the one-time mkdtempSync assumption in constructor (or
keep a nullable tempDir) and call
fs.mkdtempSync(path.join(os.tmpdir(),'subminer-kwin-')) to set this.tempDir and
recompute this.scriptPath prior to writing the script; keep stopAsync()/stop()
cleaning it up but ensure startAsync() guards/creates the directory and
scriptPath so restart does not hit ENOENT.
- Around line 396-406: The parsed payload may contain a non-array windows field
which causes selectKWinMpvWindow to throw; in handleUpdate, ensure
parsed.windows is an Array before passing it to selectKWinMpvWindow (e.g., const
windows = Array.isArray(parsed.windows) ? parsed.windows : []; then call
selectKWinMpvWindow(windows, { targetMpvSocketPath: this.targetMpvSocketPath,
getWindowCommandLine: (pid) => this.getWindowCommandLine(pid) }); apply the same
Array.isArray validation to the other selectKWinMpvWindow invocation in this
method so malformed session-bus input cannot break tracking).
---
Nitpick comments:
In `@launcher/mpv.ts`:
- Around line 228-250: The detectBackend function in launcher/mpv.ts duplicates
KWin/Wayland probing that exists in src/window-trackers/index.ts, causing drift
(notably SWAYSOCK handling only present in the tracker); extract the
session-environment detection into a shared helper (e.g., export a new
detectSessionBackend or move logic into an existing util) that centralizes
checks for XDG_CURRENT_DESKTOP, XDG_SESSION_DESKTOP, XDG_SESSION_TYPE,
WAYLAND_DISPLAY, HYPRLAND_INSTANCE_SIGNATURE, SWAYSOCK, and
commandExists('hyprctl'), then have detectBackend call that shared helper and
update src/window-trackers/index.ts to import and use it so both launcher
(detectBackend) and the overlay/tracker use identical autodetection logic.
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 121-149: The selectKWinMpvWindow hot path calls
options.getWindowCommandLine for every candidate window (when
targetMpvSocketPath is set), which currently synchronously spawns ps per PID and
blocks the main thread; change this by introducing a short-lived PID->cmdline
cache (or read /proc/<pid>/cmdline directly) and use that cache inside
selectKWinMpvWindow and the other similar block (around lines 434-443) so you
avoid spawning ps per window; update selectKWinMpvWindow to query the cache
(e.g., getCachedCmdline(pid)) before falling back to an async-safe read, and
ensure cache entries expire/refresh appropriately to keep behavior correct while
removing synchronous ps calls from the KWin update hot path.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85c041e4-f41f-48dc-b25e-0245c69ef162
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockchanges/kwin-wayland.mdis excluded by!changes/**
📒 Files selected for processing (15)
launcher/config/args-normalizer.tslauncher/config/cli-parser-builder.tslauncher/main.test.tslauncher/mpv.test.tslauncher/mpv.tslauncher/types.tspackage.jsonplugin/subminer.confplugin/subminer/environment.luaplugin/subminer/process.luascripts/test-plugin-environment.luasrc/cli/help.tssrc/window-trackers/index.tssrc/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/window-trackers/kwin-tracker.ts (2)
313-321: Synchronousstart()/stop()wrappers satisfy the abstract contract but fire-and-forget async operations.The
BaseWindowTrackerabstract class requires synchronousstart()andstop()methods. The current implementation correctly satisfies this contract by callingvoid this.startAsync()andvoid this.stopAsync(). However, callers have no way to await completion or handle errors from the async operations.This is a design tradeoff—consider documenting that
start()returns before initialization completes, or exposingstartAsync()/stopAsync()for callers that need to await.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 313 - 321, The synchronous start()/stop() in kwin-tracker.ts satisfy BaseWindowTracker but perform fire-and-forget calls to startAsync()/stopAsync(), leaving callers unable to await completion or catch errors; update the implementation and API by either (A) documenting in the class and method comments on BaseWindowTracker and in kwin-tracker.ts that start()/stop() return immediately and callers should use the existing startAsync()/stopAsync() to await/handle errors, or (B) change the public contract to expose and recommend the async methods (startAsync/stopAsync) for consumers and ensure start()/stop() remain simple wrappers that set stopped flag and log the start of async operation; make sure to reference and update BaseWindowTracker, start(), stop(), startAsync(), and stopAsync() in your changes so callers know which methods to call to await completion and handle errors.
467-474:execSyncin fallback path blocks the event loop.
readProcessCommandLineuses synchronousexecSyncfor thepsfallback when/procis unavailable. While this is only a fallback path, it could briefly block the event loop.Consider using async
execif this becomes a concern in practice, though the current implementation is acceptable given the 1-second cache TTL limits call frequency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 467 - 474, readProcessCommandLine currently calls execSync in the fallback branch which blocks the event loop; change this to use the async exec (or util.promisify(exec)) so the fallback is non-blocking, update readProcessCommandLine's signature/await call sites accordingly, and keep the same behavior of returning the trimmed command line or null on error (reference readProcessCommandLine and the execSync call).scripts/test-plugin-process-overrides.lua (1)
32-33: Consider adding a test case forkwinbackend override.The test validates
backend=windows, but since this PR also addskwinto the allowlist, adding a similar assertion would improve coverage:local kwin_overrides = process.parse_start_script_message_overrides("backend=kwin") assert_equals("kwin", kwin_overrides.backend, "expected backend=kwin override to be accepted")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-plugin-process-overrides.lua` around lines 32 - 33, Add a test for the newly allowed "kwin" backend by calling process.parse_start_script_message_overrides with "backend=kwin" and asserting the returned table's backend equals "kwin"; update the test file's existing pattern used for "windows" (see process.parse_start_script_message_overrides and the assert_equals invocation) to include a new kwin_overrides variable and an assert_equals("kwin", kwin_overrides.backend, ...) line mirroring the windows test so coverage includes the kwin allowlist case.src/shared/backend-detection.ts (1)
22-24: Fallback tox11on bareplatform === 'linux'may be overly broad.Line 23 returns
'x11'whenplatform === 'linux'even withoutDISPLAYset. This could incorrectly selectx11on headless Linux systems or TTY sessions where no display server is running.Consider tightening the condition to require
DISPLAY:- if (env.DISPLAY || platform === 'linux') return 'x11'; + if (env.DISPLAY) return 'x11';Alternatively, if the intent is to assume X11 as a last-resort default on Linux, document this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/backend-detection.ts` around lines 22 - 24, The fallback currently returns 'x11' when env.DISPLAY is falsy but platform === 'linux' (in the block that uses hasWayland, isKWinDesktop, env.DISPLAY, platform), which can misidentify headless/TTY systems; change the condition so X11 is only chosen when a DISPLAY is present (e.g., replace "if (env.DISPLAY || platform === 'linux') return 'x11';" with a stricter check like "if (env.DISPLAY) return 'x11';"), or if you truly intend a Linux default, add a clear comment/documentation and make the default explicit (e.g., a configurable fallback flag) so the behavior is intentional and discoverable.launcher/mpv.test.ts (1)
77-100: Consider extractingwithEnvhelper to a shared test utility.This helper is duplicated verbatim in
kwin-tracker.test.ts. Extracting it to a shared test utility module would reduce duplication and ensure consistent behavior across test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launcher/mpv.test.ts` around lines 77 - 100, The withEnv helper is duplicated across tests; extract the function withEnv into a shared test utility module (e.g., testUtils or helpers) and export it, then replace the inline implementations in both test files with an import of withEnv; ensure the exported signature and behavior (overrides: Record<string,string|undefined>, callback: ()=>T) stay identical and update imports in mpv.test.ts and the other test that had the duplicate so both use the single shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@launcher/mpv.test.ts`:
- Around line 77-100: The withEnv helper is duplicated across tests; extract the
function withEnv into a shared test utility module (e.g., testUtils or helpers)
and export it, then replace the inline implementations in both test files with
an import of withEnv; ensure the exported signature and behavior (overrides:
Record<string,string|undefined>, callback: ()=>T) stay identical and update
imports in mpv.test.ts and the other test that had the duplicate so both use the
single shared implementation.
In `@scripts/test-plugin-process-overrides.lua`:
- Around line 32-33: Add a test for the newly allowed "kwin" backend by calling
process.parse_start_script_message_overrides with "backend=kwin" and asserting
the returned table's backend equals "kwin"; update the test file's existing
pattern used for "windows" (see process.parse_start_script_message_overrides and
the assert_equals invocation) to include a new kwin_overrides variable and an
assert_equals("kwin", kwin_overrides.backend, ...) line mirroring the windows
test so coverage includes the kwin allowlist case.
In `@src/shared/backend-detection.ts`:
- Around line 22-24: The fallback currently returns 'x11' when env.DISPLAY is
falsy but platform === 'linux' (in the block that uses hasWayland,
isKWinDesktop, env.DISPLAY, platform), which can misidentify headless/TTY
systems; change the condition so X11 is only chosen when a DISPLAY is present
(e.g., replace "if (env.DISPLAY || platform === 'linux') return 'x11';" with a
stricter check like "if (env.DISPLAY) return 'x11';"), or if you truly intend a
Linux default, add a clear comment/documentation and make the default explicit
(e.g., a configurable fallback flag) so the behavior is intentional and
discoverable.
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 313-321: The synchronous start()/stop() in kwin-tracker.ts satisfy
BaseWindowTracker but perform fire-and-forget calls to startAsync()/stopAsync(),
leaving callers unable to await completion or catch errors; update the
implementation and API by either (A) documenting in the class and method
comments on BaseWindowTracker and in kwin-tracker.ts that start()/stop() return
immediately and callers should use the existing startAsync()/stopAsync() to
await/handle errors, or (B) change the public contract to expose and recommend
the async methods (startAsync/stopAsync) for consumers and ensure start()/stop()
remain simple wrappers that set stopped flag and log the start of async
operation; make sure to reference and update BaseWindowTracker, start(), stop(),
startAsync(), and stopAsync() in your changes so callers know which methods to
call to await completion and handle errors.
- Around line 467-474: readProcessCommandLine currently calls execSync in the
fallback branch which blocks the event loop; change this to use the async exec
(or util.promisify(exec)) so the fallback is non-blocking, update
readProcessCommandLine's signature/await call sites accordingly, and keep the
same behavior of returning the trimmed command line or null on error (reference
readProcessCommandLine and the execSync call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b2289623-ba61-4b44-85c9-3f4ce19c5c56
📒 Files selected for processing (9)
launcher/mpv.test.tslauncher/mpv.tspackage.jsonplugin/subminer/process.luascripts/test-plugin-process-overrides.luasrc/shared/backend-detection.tssrc/window-trackers/index.tssrc/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Owner
|
I found a KWin crash that happens intermittently with the new KDE Wayland tracker. It does not seem to be a general playback issue. Normal playback usually works. The crash appears to happen when KWin emits Most suspicious path is the generated KWin script in Latest repro was taking a screenshot during playback, so screenshot/snipping UI is at least one trigger. Suggested fix direction: add strict early filtering before any |
Owner
|
I encountered this issue because I was trying to take a screenshot of another small bug in the invisible window positioning. I can't provide the screenshot because kwin won't let me take the screenshot, but if you have the plugin installed and press y-d chord on the video you can see the window is slightly offset from mpv |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/window-trackers/kwin-tracker.ts (2)
184-202: Consider wrapping property access in try-catch for additional crash resilience.The
isWatchableWindowguard correctly filters before WeakSet access, addressing the reported crash. However, the PR discussion suggests that even reading properties likewindow.managedorwindow.deletedon certain internal/transient KWin windows can trigger SIGSEGVs.For maximum resilience, consider wrapping the property reads in a try-catch:
🛡️ Optional defensive enhancement
function isWatchableWindow(window) { if (!window || typeof window !== "object") { return false; } + try { if (window.managed === false || window.deleted === true) { return false; } if (window.normalWindow !== true) { return false; } if (window.specialWindow === true || window.transient === true) { return false; } if (window.popupWindow === true || window.outline === true) { return false; } - return true; + } catch (e) { + return false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 184 - 202, The isWatchableWindow guard should defensively handle potential crashes when reading properties from exotic KWin window objects: wrap the series of property reads (window.managed, window.deleted, window.normalWindow, window.specialWindow, window.transient, window.popupWindow, window.outline) in a try-catch and return false if any exception is thrown, preserving the existing boolean checks and final true return when all checks pass; update the isWatchableWindow function to perform the same logical checks but inside the try block so any SIGSEGV-like access errors result in a safe false without changing the guard semantics.
473-491: Add pid validation inreadProcessCommandLinefor defense in depth.Currently,
pidis validated upstream inselectKWinMpvWindow(line 140) before reaching this method. However,readProcessCommandLinedirectly interpolatespidinto a shell command without local validation. If this method is ever called from a different code path or during refactoring, it could be vulnerable to command injection.🛡️ Proposed fix
private readProcessCommandLine(pid: number): string | null { + if (!Number.isInteger(pid) || pid <= 0) { + return null; + } + if (process.platform === 'linux') { try { const commandLine = fs.readFileSync(`/proc/${pid}/cmdline`, 'utf-8').replace(/\0/g, ' ').trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 473 - 491, readProcessCommandLine currently interpolates pid into a shell command which risks command injection; add local validation and safer process invocation: validate pid is a finite positive integer (e.g., Number.isInteger(pid) && pid > 0) at the start of readProcessCommandLine and return null for invalid values, use that sanitized pid when reading /proc, and replace execSync with a non-shell invocation (e.g., child_process.execFileSync or spawnSync calling "ps" with args ['-p', pid.toString(), '-o', 'args='] and encoding 'utf-8') so the pid is passed as an argument rather than injected into a shell; reference functions readProcessCommandLine and the upstream selectKWinMpvWindow for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 184-202: The isWatchableWindow guard should defensively handle
potential crashes when reading properties from exotic KWin window objects: wrap
the series of property reads (window.managed, window.deleted,
window.normalWindow, window.specialWindow, window.transient, window.popupWindow,
window.outline) in a try-catch and return false if any exception is thrown,
preserving the existing boolean checks and final true return when all checks
pass; update the isWatchableWindow function to perform the same logical checks
but inside the try block so any SIGSEGV-like access errors result in a safe
false without changing the guard semantics.
- Around line 473-491: readProcessCommandLine currently interpolates pid into a
shell command which risks command injection; add local validation and safer
process invocation: validate pid is a finite positive integer (e.g.,
Number.isInteger(pid) && pid > 0) at the start of readProcessCommandLine and
return null for invalid values, use that sanitized pid when reading /proc, and
replace execSync with a non-shell invocation (e.g., child_process.execFileSync
or spawnSync calling "ps" with args ['-p', pid.toString(), '-o', 'args='] and
encoding 'utf-8') so the pid is passed as an argument rather than injected into
a shell; reference functions readProcessCommandLine and the upstream
selectKWinMpvWindow for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae1f48d3-696b-40cf-89ce-474a59be095d
📒 Files selected for processing (2)
src/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/window-trackers/kwin-tracker.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/window-trackers/kwin-tracker.ts (1)
177-587: Consider splittingkwin-tracker.tsinto smaller modules.This file currently mixes bridge-script generation, selection heuristics, DBus transport, and lifecycle orchestration. Breaking it into focused modules (e.g.,
kwin-bridge-script.ts,kwin-window-selection.ts,kwin-dbus-client.ts) would improve testability and future maintenance.As per coding guidelines,
**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 177 - 587, The file bundles bridge-script generation, selection logic, DBus calls and lifecycle in one large module; split responsibilities by extracting buildKWinBridgeScript into a new kwin-bridge-script.ts, moving selection logic (used by selectKWinMpvWindow/selectKWinMpvWindow helper) into kwin-window-selection.ts, and isolating DBus call/utility methods (callMethod, loadScript, runScript, stopScript, unloadScript and related constants) into kwin-dbus-client.ts that export small functions or a helper class; update KWinWindowTracker to import buildKWinBridgeScript, selectKWinMpvWindow and the DBus helpers (keep KWinWindowTracker API and private method names unchanged: ensureScriptWorkspace, startAsync, stopAsync, handleUpdate, getWindowCommandLine, readProcessCommandLine still work) and wire exports so existing callers see the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 94-99: The matchesTargetSocket function currently uses
commandLine.includes which permits prefix collisions (e.g. /tmp/mpv matching
/tmp/mpv2); update matchesTargetSocket to perform exact token matching instead:
parse or regex the commandLine to look specifically for the two forms
"--input-ipc-server=<path>" and "--input-ipc-server <path>" where <path> exactly
equals targetMpvSocketPath (allowing optional quoting around the path and
ensuring the path is followed by end-of-token like whitespace or end-of-string),
and return true only on those exact matches so it cannot match prefixes.
- Around line 426-430: parsed.windows may contain non-object or malformed
entries causing selectKWinMpvWindow to throw when accessing properties like
normalWindow; before calling selectKWinMpvWindow, sanitize and filter
parsed.windows into a well-typed array (e.g., filter out null/undefined and
non-objects, and ensure required fields like normalWindow and pid exist) and
pass that cleaned array to selectKWinMpvWindow (keep using the same options
object with this.targetMpvSocketPath and getWindowCommandLine). Also consider
early-returning or logging when no valid windows remain to avoid passing an
empty/invalid list into selectKWinMpvWindow.
---
Nitpick comments:
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 177-587: The file bundles bridge-script generation, selection
logic, DBus calls and lifecycle in one large module; split responsibilities by
extracting buildKWinBridgeScript into a new kwin-bridge-script.ts, moving
selection logic (used by selectKWinMpvWindow/selectKWinMpvWindow helper) into
kwin-window-selection.ts, and isolating DBus call/utility methods (callMethod,
loadScript, runScript, stopScript, unloadScript and related constants) into
kwin-dbus-client.ts that export small functions or a helper class; update
KWinWindowTracker to import buildKWinBridgeScript, selectKWinMpvWindow and the
DBus helpers (keep KWinWindowTracker API and private method names unchanged:
ensureScriptWorkspace, startAsync, stopAsync, handleUpdate,
getWindowCommandLine, readProcessCommandLine still work) and wire exports so
existing callers see the same symbols.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3bc7225c-7439-49d2-85b9-cb5e436b14ad
📒 Files selected for processing (2)
src/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/window-trackers/kwin-tracker.test.ts
Owner
|
KWin is no longer crashing, but I did some more testing on this branch and found we still have a couple of issues:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/window-trackers/kwin-tracker.ts (1)
604-610: Consider defensive check for empty reply body on non-void methods.If
reply.bodyis an empty array,values[0]will beundefined, which is silently cast toT. Forvoidreturn types this is fine, but forloadScript(expectingnumber) an empty body would cause subtle failures downstream.🔧 Optional defensive check
if (!reply) { throw new Error(`No reply received from ${options.interfaceName}.${options.member}`); } const values = (reply.body ?? []) as T[]; + if (values.length === 0 && options.signature) { + throw new Error(`Empty reply body from ${options.interfaceName}.${options.member}`); + } return values[0] as T;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 604 - 610, The code currently returns values[0] even when reply.body is an empty array, which yields undefined for non-void methods (e.g., loadScript expecting a number); add a defensive check after const values = (reply.body ?? []) as T[] to throw a descriptive Error when values.length === 0 for non-void calls (include options.interfaceName and options.member in the message), so callers like loadScript fail fast instead of receiving undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/shortcut-fallback.ts`:
- Line 90: The shortcut matching currently accepts both 'keyDown' and
'rawKeyDown' in shortcutMatchesInputForLocalFallback which can cause
double-firing; update shortcutMatchesInputForLocalFallback to only accept
'keyDown' (matching the behavior in isTabInputForMpvForwarding,
isLookupWindowToggleInput, isKeyboardModeToggleInput) or otherwise deduplicate
events so a single physical keypress triggers the shortcut once, ensuring the
function only returns true for input.type === 'keyDown' (and still respects
input.isAutoRepeat).
---
Nitpick comments:
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 604-610: The code currently returns values[0] even when reply.body
is an empty array, which yields undefined for non-void methods (e.g., loadScript
expecting a number); add a defensive check after const values = (reply.body ??
[]) as T[] to throw a descriptive Error when values.length === 0 for non-void
calls (include options.interfaceName and options.member in the message), so
callers like loadScript fail fast instead of receiving undefined.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d646872-3389-4083-853b-6c304a2c0365
📒 Files selected for processing (4)
src/core/services/overlay-shortcut-handler.test.tssrc/core/services/shortcut-fallback.tssrc/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/window-trackers/kwin-tracker.test.ts
Contributor
Author
|
Pushed What changed in this follow-up:
Validation used for this follow-up:
Known unrelated repo baseline failures still present in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/window-trackers/kwin-tracker.ts (4)
754-762: Fire-and-forget async patterns may swallow important errors.
start()andstop()discard the promises fromstartAsync()andstopAsync()usingvoid. While errors are logged internally, callers cannot await completion or handle failures. This is acceptable for event-driven lifecycles but worth noting.Consider returning the promise or providing a callback mechanism if callers need to know when initialization completes or fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 754 - 762, The start() and stop() methods currently discard the promises from startAsync() and stopAsync() using "void", which hides completion and failures; change the signatures of start() and stop() to return the Promise from startAsync()/stopAsync() (or alternatively accept a callback/emit an event) so callers can await or handle errors from startAsync()/stopAsync(); update references to start() and stop() accordingly and ensure any internal error logging remains but do not swallow the returned promise from startAsync()/stopAsync().
916-923:execFileSyncblocks the event loop.
readProcessCommandLineusesexecFileSyncwhich blocks. While the TTL cache (1 second) limits how often this runs, consider usingexecFilewith a callback or promisified version for non-blocking I/O, especially if multiple mpv windows are being tracked.♻️ Optional: async version using execFile
+import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); + - private readProcessCommandLine(pid: number): string | null { + private async readProcessCommandLine(pid: number): Promise<string | null> { if (!Number.isInteger(pid) || pid <= 0) { return null; } const safePid = String(pid); if (process.platform === 'linux') { try { const commandLine = fs .readFileSync(`/proc/${safePid}/cmdline`, 'utf-8') .replace(/\0/g, ' ') .trim(); return commandLine || null; } catch { // fall through to ps for environments without /proc access } } try { - const commandLine = execFileSync('ps', ['-p', safePid, '-o', 'args='], { - encoding: 'utf-8', - }).trim(); + const { stdout } = await execFileAsync('ps', ['-p', safePid, '-o', 'args='], { + encoding: 'utf-8', + }); + const commandLine = stdout.trim(); return commandLine || null; } catch { return null; } }Note: This would require making
getWindowCommandLineasync as well, which cascades toselectKWinMpvWindow. Given the caching and typical use case, the current sync implementation is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 916 - 923, readProcessCommandLine currently uses execFileSync which blocks the event loop; change it to a non-blocking variant (use child_process.execFile with a callback or util.promisify(execFile) returning a Promise) and update readProcessCommandLine to be async and return the command line or null asynchronously. Propagate async changes to getWindowCommandLine (make it async and await readProcessCommandLine) and then to selectKWinMpvWindow so callers await the result; preserve the 1s TTL cache behavior by storing Promises or caching resolved values appropriately to avoid duplicate concurrent execs. Ensure error handling mirrors the current behavior by returning null on failure and keep function names readProcessCommandLine, getWindowCommandLine, and selectKWinMpvWindow to locate the changes.
186-730: Consider extracting the KWin bridge script to a separate file.The inline script is ~540 lines, making this file quite large (~1020 lines total). Extracting it to a separate
.jstemplate file would improve maintainability and enable syntax highlighting/linting for the KWin script code.As per coding guidelines: "TypeScript files should be kept small and follow existing patterns."
♻️ Suggested approach
- Create
src/window-trackers/kwin-bridge-script.template.jswith the script content- Use template literals or a simple placeholder replacement for dynamic values (
SERVICE_NAME,OBJECT_PATH, etc.)- Read and interpolate at runtime:
// In kwin-tracker.ts import { readFileSync } from 'node:fs'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; const SCRIPT_TEMPLATE_PATH = join(dirname(fileURLToPath(import.meta.url)), 'kwin-bridge-script.template.js'); export function buildKWinBridgeScript(serviceName: string): string { const template = readFileSync(SCRIPT_TEMPLATE_PATH, 'utf-8'); return template .replace('__SERVICE_NAME__', JSON.stringify(serviceName)) .replace('__OBJECT_PATH__', JSON.stringify(BRIDGE_OBJECT_PATH)) .replace('__INTERFACE_NAME__', JSON.stringify(BRIDGE_INTERFACE_NAME)) .replace('__OVERLAY_OWNER_PID__', JSON.stringify(process.pid)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 186 - 730, The buildKWinBridgeScript function currently inlines a ~540-line KWin script making the TypeScript file huge; extract the script into a separate template file (e.g., kwin-bridge-script.template.js) and update buildKWinBridgeScript to load and interpolate the dynamic placeholders (SERVICE_NAME, OBJECT_PATH/BRIDGE_OBJECT_PATH, INTERFACE_NAME/BRIDGE_INTERFACE_NAME, OVERLAY_OWNER_PID/process.pid) at runtime; ensure placeholders in the template match the replacement tokens used by buildKWinBridgeScript and preserve existing behavior of functions like emitState, syncOverlayPairState, watchWindow, etc., while using readFileSync (or equivalent) to return the final script string.
1013-1018: Unsafe type assertion on D-Bus reply body.The
callMethodhelper castsreply.bodytoT[]and returnsvalues[0], but if the reply has an empty body or the method returnsvoid, this returnsundefinedcast asT. For void-returning methods likerun/stop, this works, but it's fragile.♻️ Safer handling
private async callMethod<T>( bus: MessageBus, options: { path: string; interfaceName: string; member: string; signature?: string; body?: unknown[]; }, ): Promise<T> { const reply = await bus.call( new dbus.Message({ destination: KWIN_SERVICE_NAME, path: options.path, interface: options.interfaceName, member: options.member, signature: options.signature ?? '', body: options.body ?? [], }), ); if (!reply) { throw new Error(`No reply received from ${options.interfaceName}.${options.member}`); } - const values = (reply.body ?? []) as T[]; - return values[0] as T; + const values = reply.body ?? []; + return (Array.isArray(values) && values.length > 0 ? values[0] : undefined) as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/kwin-tracker.ts` around lines 1013 - 1018, The current code in callMethod (the block that reads reply.body into values and returns values[0]) unsafely casts an empty or void D-Bus reply to T; change the logic to explicitly handle an absent/empty reply.body by checking if (!reply.body || (Array.isArray(reply.body) && reply.body.length === 0)) and then either return undefined (update the callMethod signature to return Promise<T | undefined>) or throw a clear error depending on the call site expectations; update callers of callMethod that expect void-returning methods (e.g., run/stop) to accept the new undefined/void result or to assert presence only when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 128-133: Update the Linux row in the platform requirements table
in README.md to explicitly include Sway as a supported compositor (alongside KDE
Plasma Wayland and Hyprland), note that Sway support relies on the SWAYSOCK
environment variable (per backend-detection.ts) and that users typically need
the swaymsg helper binary for window tracking (analogous to hyprctl for Hyprland
mentioned in args-normalizer.ts); make the table entry concise and add a short
footnote or parenthetical indicating "swaymsg required" so Sway users know the
extra dependency.
---
Nitpick comments:
In `@src/window-trackers/kwin-tracker.ts`:
- Around line 754-762: The start() and stop() methods currently discard the
promises from startAsync() and stopAsync() using "void", which hides completion
and failures; change the signatures of start() and stop() to return the Promise
from startAsync()/stopAsync() (or alternatively accept a callback/emit an event)
so callers can await or handle errors from startAsync()/stopAsync(); update
references to start() and stop() accordingly and ensure any internal error
logging remains but do not swallow the returned promise from
startAsync()/stopAsync().
- Around line 916-923: readProcessCommandLine currently uses execFileSync which
blocks the event loop; change it to a non-blocking variant (use
child_process.execFile with a callback or util.promisify(execFile) returning a
Promise) and update readProcessCommandLine to be async and return the command
line or null asynchronously. Propagate async changes to getWindowCommandLine
(make it async and await readProcessCommandLine) and then to selectKWinMpvWindow
so callers await the result; preserve the 1s TTL cache behavior by storing
Promises or caching resolved values appropriately to avoid duplicate concurrent
execs. Ensure error handling mirrors the current behavior by returning null on
failure and keep function names readProcessCommandLine, getWindowCommandLine,
and selectKWinMpvWindow to locate the changes.
- Around line 186-730: The buildKWinBridgeScript function currently inlines a
~540-line KWin script making the TypeScript file huge; extract the script into a
separate template file (e.g., kwin-bridge-script.template.js) and update
buildKWinBridgeScript to load and interpolate the dynamic placeholders
(SERVICE_NAME, OBJECT_PATH/BRIDGE_OBJECT_PATH,
INTERFACE_NAME/BRIDGE_INTERFACE_NAME, OVERLAY_OWNER_PID/process.pid) at runtime;
ensure placeholders in the template match the replacement tokens used by
buildKWinBridgeScript and preserve existing behavior of functions like
emitState, syncOverlayPairState, watchWindow, etc., while using readFileSync (or
equivalent) to return the final script string.
- Around line 1013-1018: The current code in callMethod (the block that reads
reply.body into values and returns values[0]) unsafely casts an empty or void
D-Bus reply to T; change the logic to explicitly handle an absent/empty
reply.body by checking if (!reply.body || (Array.isArray(reply.body) &&
reply.body.length === 0)) and then either return undefined (update the
callMethod signature to return Promise<T | undefined>) or throw a clear error
depending on the call site expectations; update callers of callMethod that
expect void-returning methods (e.g., run/stop) to accept the new undefined/void
result or to assert presence only when appropriate.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a7386660-9be8-4f1c-999b-2c457a145d85
⛔ Files ignored due to path filters (3)
changes/kwin-wayland.mdis excluded by!changes/**docs-site/installation.mdis excluded by!docs-site/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (3)
README.mdsrc/window-trackers/kwin-tracker.test.tssrc/window-trackers/kwin-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/window-trackers/kwin-tracker.test.ts
bee-san
force-pushed
the
codex/kde-plasma-wayland
branch
from
5a90453 to
eec17a1
Compare
Contributor
Author
|
Rebased this PR by rebuilding the reviewed tree cleanly on top of current Local validation on the rebuilt branch:
|
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.
What changed
This PR adds KDE Plasma Wayland support across the launcher, mpv plugin, CLI/backend validation, and the Electron window-tracker registry.
It introduces a new
KWinWindowTrackerthat loads a small runtime KWin script over session DBus, receives mpv window geometry and focus updates from KWin, and preserves the existing mpv socket matching behavior so the overlay still follows the correct player window when multiple mpv instances exist.It also fixes the follow-up issues found in review:
autobackend selection is no longer forced through mpv script opts, and the KWin compatibility fallback now unloads scripts correctly on shutdown.Why
SubMiner already had compositor-aware Linux tracking for Hyprland and Sway, but KDE Plasma Wayland sessions were still falling back to the wrong backend path. In practice that meant the overlay could not reliably track native Wayland mpv windows on Plasma.
Review also surfaced two regressions in the initial KWin work: mpv auto-start flows could mis-detect Hyprland, and older Plasma fallback loading could leak KWin scripts across restarts.
User impact
KDE Plasma Wayland users can now run SubMiner without forcing an unrelated backend, and the overlay can follow native Wayland mpv windows on Plasma.
There is also a new explicit
kwinbackend option anywhere the project already accepts backend overrides.Validation
bun run typecheckHOME=/tmp/subminer-codex-home XDG_CONFIG_HOME=/tmp/subminer-codex-xdg bun test launcher/mpv.test.ts launcher/main.test.ts src/window-trackers/kwin-tracker.test.tsbun run test:plugin:srcSummary by CodeRabbit
New Features
Tests
Documentation
kwin) as supported.Chores