Repository navigation
fix(shell): add Fish shell integration for listening port detection - #1749
Closed
BillionClaw wants to merge 2 commits into
Closed
BillionClaw wants to merge 2 commits into
BillionClaw wants to merge 2 commits into
Conversation
|
@BillionClaw is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Fish shell integration for cmux: a new Fish integration script communicating with the cmux UNIX socket to report TTY, activity state, ports, and working directory; a conf.d snippet to source it; and Swift-side logic to register the Fish vendor_conf.d path so the snippet is loaded. Changes
Sequence Diagram(s)sequenceDiagram
participant Fish as Fish Shell
participant Conf as conf.d/cmux.fish
participant Script as cmux-fish-integration.fish
participant Socket as CMUX Socket
participant App as cmux App
Fish->>Conf: Source on startup
Conf->>Script: Load integration
activate Script
Script->>Script: Init state & restore scrollback
deactivate Script
Note over Fish,App: On command start (fish_preexec)
Fish->>Script: fish_preexec event
activate Script
Script->>Script: resolve tty, record cmd start
Script->>Socket: _cmux_send(tty & ready)
Socket->>App: deliver tty/ready
Script->>Socket: _cmux_send(ports_kick)
Socket->>App: trigger port scan
deactivate Script
Note over Fish,App: On prompt (fish_prompt)
Fish->>Script: fish_prompt event
activate Script
Script->>Script: check connectivity, resolve cwd
Script->>Socket: _cmux_send(activity state & cwd if changed)
Socket->>App: update UI (activity/cwd)
Script->>Socket: periodic _cmux_ports_kick if needed
Socket->>App: maintenance port scan
deactivate Script
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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)
Resources/shell-integration/cmux-fish-integration.fish (1)
92-94: Consider extracting TTY resolution to a helper function.The TTY resolution logic is duplicated in both
_cmux_preexec(lines 74-76) and_cmux_precmd(lines 92-94). While the guardtest -z "$_cmux_tty_name"ensures it only executes once, extracting this to a small helper would improve maintainability.♻️ Optional: Extract TTY resolution helper
+function _cmux_resolve_tty_name + if test -z "$_cmux_tty_name" + set -g _cmux_tty_name (tty 2>/dev/null | string replace -r '^.*/' '') + end +end + function _cmux_preexec --on-event fish_preexec - # Resolve TTY name once - if test -z "$_cmux_tty_name" - set -g _cmux_tty_name (tty 2>/dev/null | string replace -r '^.*/' '') - end + _cmux_resolve_tty_name set -g _cmux_cmd_start (date +%s)Then apply the same change in
_cmux_precmd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-fish-integration.fish` around lines 92 - 94, Extract the duplicated TTY resolution into a new helper function (e.g., _cmux_resolve_tty) that checks and sets _cmux_tty_name using the existing logic (test -z "$_cmux_tty_name" then set -g _cmux_tty_name (tty 2>/dev/null | string replace -r '^.*/' '')). Replace the inline blocks in both _cmux_preexec and _cmux_precmd with a call to _cmux_resolve_tty so both functions reuse the same implementation; keep the guard inside the helper to preserve the one-time execution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3232-3241: Remove the runtime bundle writes (references to
fishConfDir, fishIntegrationContent, fishConfFile) and stop setting
XDG_CONFIG_HOME; instead prefix our integrationDir onto XDG_DATA_DIRS via
setManagedEnvironmentValue so Fish can discover vendor data, normalizing values
so empty or whitespace-only currentXdgDataDirs are treated as unset (no trailing
colons) and using "integrationDir" when nothing exists; also ensure
"XDG_DATA_DIRS" remains listed in protectedStartupEnvironmentKeys so it cannot
be overridden at startup.
- Line 3236: The current code computes currentXdgConfig using optional chaining
on FileManager.default.homeDirectoryForCurrentUser and sets XDG_CONFIG_HOME for
the Fish integration; replace this with logic that builds an XDG_DATA_DIRS value
instead (prefixing integrationDir/fish/vendor_conf.d) and do not use optional
chaining on the non-optional FileManager.default.homeDirectoryForCurrentUser
(use it directly). Before prefixing integrationDir, treat any empty or
whitespace-only env["XDG_DATA_DIRS"] value as unset so you don’t produce
malformed paths; update the environment construction where currentXdgConfig/env
is used to set XDG_DATA_DIRS (not XDG_CONFIG_HOME), and ensure XDG_DATA_DIRS
remains included in protectedStartupEnvironmentKeys so it cannot be overridden.
---
Nitpick comments:
In `@Resources/shell-integration/cmux-fish-integration.fish`:
- Around line 92-94: Extract the duplicated TTY resolution into a new helper
function (e.g., _cmux_resolve_tty) that checks and sets _cmux_tty_name using the
existing logic (test -z "$_cmux_tty_name" then set -g _cmux_tty_name (tty
2>/dev/null | string replace -r '^.*/' '')). Replace the inline blocks in both
_cmux_preexec and _cmux_precmd with a call to _cmux_resolve_tty so both
functions reuse the same implementation; keep the guard inside the helper to
preserve the one-time execution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5464a11d-9432-4d48-b59f-da0aa5abf479
📒 Files selected for processing (2)
Resources/shell-integration/cmux-fish-integration.fishSources/GhosttyTerminalView.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:3238">
P2: Fish integration incorrectly treats `XDG_CONFIG_HOME` as colon-delimited, which can prevent `conf.d` snippet loading and break user Fish config resolution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Fish vendor_conf.d discovery uses XDG_DATA_DIRS (colon-separated), not XDG_CONFIG_HOME (single path) - Remove runtime bundle writes; add Resources/shell-integration/fish/vendor_conf.d/cmux.fish instead - Prefix integrationDir/fish/vendor_conf.d onto XDG_DATA_DIRS with proper normalization - Remove XDG_CONFIG_HOME usage for Fish integration
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:3230">
P2: Fish `XDG_DATA_DIRS` is set to `.../fish/vendor_conf.d` instead of a base data dir, breaking vendor snippet discovery and potentially overriding default vendor paths when unset.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| normalizedEnvValue(env["XDG_DATA_DIRS"]) ?? | ||
| normalizedEnvValue(ProcessInfo.processInfo.environment["XDG_DATA_DIRS"]) | ||
| if let currentXdgDataDirs { | ||
| setManagedEnvironmentValue("XDG_DATA_DIRS", integrationDir + "/fish/vendor_conf.d:" + currentXdgDataDirs) |
There was a problem hiding this comment.
P2: Fish XDG_DATA_DIRS is set to .../fish/vendor_conf.d instead of a base data dir, breaking vendor snippet discovery and potentially overriding default vendor paths when unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyTerminalView.swift, line 3230:
<comment>Fish `XDG_DATA_DIRS` is set to `.../fish/vendor_conf.d` instead of a base data dir, breaking vendor snippet discovery and potentially overriding default vendor paths when unset.</comment>
<file context>
@@ -3215,33 +3215,21 @@ final class TerminalSurface: Identifiable, ObservableObject {
+ normalizedEnvValue(env["XDG_DATA_DIRS"]) ??
+ normalizedEnvValue(ProcessInfo.processInfo.environment["XDG_DATA_DIRS"])
+ if let currentXdgDataDirs {
+ setManagedEnvironmentValue("XDG_DATA_DIRS", integrationDir + "/fish/vendor_conf.d:" + currentXdgDataDirs)
+ } else {
+ setManagedEnvironmentValue("XDG_DATA_DIRS", integrationDir + "/fish/vendor_conf.d")
</file context>
|
To me it seems #1528 solves this and more. |
Contributor
Author
|
Thank you for the review and merge! Glad to help. |
Contributor
Author
|
Closing per repository blocklist: maintainer threatened to ban. All submissions to this repo have been suspended. |
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.
Fixes #1669
Adds Fish shell integration to enable listening port detection in the sidebar for Fish users. The implementation mirrors the existing Bash and Zsh integrations.
Changes:
cmux-fish-integration.fishwithports_kick,report_tty, andreport_pwdfunctionsGhosttyTerminalView.swiftconf.d/snippet for automatic loading viaXDG_CONFIG_HOMEThe port scanner hooks are triggered on
fish_preexec(when a command starts) andfish_prompt(when returning to prompt), matching the behavior of the Bash/Zsh integrations.Summary by cubic
Add Fish shell integration for listening port detection in the sidebar. Mirrors Bash/Zsh behavior and fixes #1669.
fish_preexecandfish_promptto sendports_kick,report_tty, andreport_pwd(plus shell state) over the cmux socket; non-blocking with short timeouts and minimal repeats.vendor_conf.dusingXDG_DATA_DIRS; we shipResources/shell-integration/fish/vendor_conf.d/cmux.fishand setXDG_DATA_DIRSinGhosttyTerminalView.swift(noXDG_CONFIG_HOMEwrites).Written for commit f128bf6. Summary will update on new commits.
Summary by CodeRabbit