Add Ghostty extension - #283
Conversation
|
@clankus-aurelius review |
|
Thanks for contributing an extension to Vicinae! 👋 Before publication, this pull request receives two reviews:
🔴 Contributor changes requested. Address the blocking inline findings and push a new commit; the bot will review it automatically. 1 blocking finding must be addressed. The automated reviewer examines only the current commit. New commits invalidate its previous decision and start another review. |
clankus-aurelius
left a comment
There was a problem hiding this comment.
The new-tab command has a blocking silent-failure path. Five additional correctness and error-reporting problems affect window targeting, launch configurations, path handling, and fallback launching.
Automated review found 1 publication-blocking issue.
| try { | ||
| focusGhosttyWithKWin(); | ||
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250); | ||
| sendNewTabShortcut(); |
There was a problem hiding this comment.
🔴 Blocking — Missing ydotool socket is treated as success
Rule: CORRECTNESS-001
sendNewTabShortcut returns false when the socket is absent, but the result is ignored. The command then exits without creating a tab, opening Ghostty, or showing feedback.
Suggested resolution: Treat a false result as a failure so the existing fallback runs.
| sendNewTabShortcut(); | |
| if (!sendNewTabShortcut()) throw new Error("ydotool socket not found"); |
| const cap = String(w.caption || '').toLowerCase(); | ||
| if (klass.includes('ghostty') || cap.includes('ghostty')) { |
There was a problem hiding this comment.
🟠 Warning — Window caption can select an unrelated application
Rule: CORRECTNESS-001
The KWin script accepts any window whose caption contains “ghostty”. A browser or editor displaying Ghostty-related content can therefore receive Ctrl+Shift+T instead of Ghostty.
Suggested resolution: Identify Ghostty only from its application/resource class, not its caption.
| const cap = String(w.caption || '').toLowerCase(); | |
| if (klass.includes('ghostty') || cap.includes('ghostty')) { | |
| if (klass.includes('ghostty')) { |
| export function focusWindow(id: string): void { execFileSync("wmctrl", ["-ia", id], { timeout: 3000 }); } | ||
| export function listGhosttyWindows(): { id: string; title: string }[] { | ||
| const out = execFileSync("wmctrl", ["-lx"], { encoding: "utf8", timeout: 3000 }); | ||
| return out.split(/\n/).filter(l => /ghostty/i.test(l)).map(l => { const parts = l.trim().split(/\s+/); return { id: parts[0], title: parts.slice(4).join(" ") || "Ghostty" }; }); |
There was a problem hiding this comment.
🟠 Warning — Window search matches titles instead of application class
Rule: CORRECTNESS-001
The filter tests the entire wmctrl line, including the window title, so unrelated windows with “Ghostty” in their title appear in the results and can be focused.
Suggested resolution: Parse the wmctrl application-class field and filter that field for Ghostty before constructing list items.
| const out = execFileSync("wmctrl", ["-lx"], { encoding: "utf8", timeout: 3000 }); | ||
| return out.split(/\n/).filter(l => /ghostty/i.test(l)).map(l => { const parts = l.trim().split(/\s+/); return { id: parts[0], title: parts.slice(4).join(" ") || "Ghostty" }; }); | ||
| } | ||
| export function resolveOpenPath(input?: string): string { const p = expandHome((input || "").trim() || homedir()); try { const st = lstatSync(p); return st.isDirectory() ? resolve(p) : dirname(resolve(p)); } catch { return homedir(); } } |
There was a problem hiding this comment.
🟠 Warning — Invalid paths silently open the home directory
Rule: CORRECTNESS-001
Any nonexistent or inaccessible supplied path is converted to the home directory, so a typo causes “Open with Ghostty” to open the wrong location without feedback.
Suggested resolution: Let path lookup failures propagate to the command’s existing failure toast instead of substituting the home directory.
| export function resolveOpenPath(input?: string): string { const p = expandHome((input || "").trim() || homedir()); try { const st = lstatSync(p); return st.isDirectory() ? resolve(p) : dirname(resolve(p)); } catch { return homedir(); } } | |
| export function resolveOpenPath(input?: string): string { | |
| const p = expandHome((input || "").trim() || homedir()); | |
| const st = lstatSync(p); | |
| return st.isDirectory() ? resolve(p) : dirname(resolve(p)); | |
| } |
| const safe = name.toLowerCase().replace(/[^a-z0-9._-]+/g, "-").replace(/^-|-$/g, "") || "launch-config"; | ||
| const path = join(launchConfigDir, `${safe}.yaml`); | ||
| const parsed = YAML.parse(yaml); | ||
| if (!parsed || !Array.isArray(parsed.windows)) throw new Error("Launch config must include a windows list"); |
There was a problem hiding this comment.
🟠 Warning — Launch configuration validation accepts malformed command fields
Rule: CORRECTNESS-001
Validation checks only that windows is an array. For example, a scalar commands value is saved successfully, then collectCommands spreads it into individual characters and executes the wrong command sequence.
Suggested resolution: Validate the nested windows, tabs, layouts, panes, cwd, and commands types when loading and saving configurations; require commands to be arrays of strings.
| const child = spawn(bin, [], { detached: true, stdio: "ignore", cwd: process.env.HOME || undefined }); | ||
| child.unref(); | ||
| await showToast({ style: Toast.Style.Failure, title: "Could not send new-tab shortcut", message: "Opened Ghostty instead. Check qdbus, KWin scripting, and ydotool." }); |
There was a problem hiding this comment.
🟠 Warning — Fallback reports Ghostty opened before spawn succeeds
Rule: UX-001
spawn reports a missing or non-executable binary asynchronously, so this try/catch can still show “Opened Ghostty instead” and leave the process error unhandled when no window was opened.
Suggested resolution: Use the existing checked Ghostty spawning helper or await the child spawn/error event before reporting the fallback outcome.
|
For the window focus code, did you think about using the Window Management API? |
Adds a Linux Ghostty extension for Vicinae.
Features:
Verification: