Repository navigation
Conversation
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughConverted brace-group-plus- Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a UX annoyance in the bash shell integration where short-lived background tasks (shell-state sync, port detection, cwd tracking, tty reporting) were emitting Changes:
Notes:
Confidence Score: 5/5Safe to merge — targeted, well-understood bash idiom swap with no functional changes. All four changed call sites use the same correct No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Bash Shell (PROMPT_COMMAND)
participant Subshell as ( ) Subshell
participant BgProc as _cmux_send (background)
participant App as cmux App (socket)
Shell->>Subshell: fork subshell ( _cmux_send ... & )
activate Subshell
Subshell->>BgProc: start & (orphan, no job tracking)
Subshell-->>Shell: exits immediately
deactivate Subshell
Note over Shell: No job number registered<br/>No [n] Done ... notification
BgProc->>App: send socket message (report_pwd / report_shell_state / ports_kick / tty)
BgProc-->>BgProc: stdout/stderr → /dev/null
BgProc-->>BgProc: exits (reparented to init, untracked)
Reviews (1): Last reviewed commit: "Fix bash job completion notifications le..." | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
The patch is not cleanly applying to the latest version of the integration script. I had to apply these manually. |
d86dfa3 to
73084b2
Compare
|
Thanks for the heads up @sandipb! I'll rebase this onto the latest integration script so the patch applies cleanly. Should update shortly. |
73084b2 to
a598d25
Compare
Replace `{ cmd; } & disown` with `( cmd & )` subshell pattern in 4
places. The outer subshell exits immediately, making the inner
background process an orphan that bash no longer tracks, preventing
spurious "[n] Done ..." messages.
Closes manaflow-ai#2494
a598d25 to
aabda91
Compare
|
Rebased onto the latest |
|
The patch does cleanly apply. Thanks so much for updating it. However I must point out that there is alternate PR from the cmux team which is working on this exact change too. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 1001-1004: In _cmux_emit_pr_command_hint(), replace the current
async send block that uses the braces/disown pattern (the `{ _cmux_send ...; }
>/dev/null 2>&1 & disown` code) with the subshell background pattern used
elsewhere: wrap the _cmux_send invocation in a subshell like `( _cmux_send ...
>/dev/null 2>&1 & )` so it is spawned in the background without causing bash
job-control “[n] Done …” notifications; ensure you preserve the same arguments
(e.g. the `report_...` string and `--tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID`)
and quoting behavior used in _cmux_emit_pr_command_hint().
🪄 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: 76e13417-7669-4656-97ed-a061557af591
📒 Files selected for processing (1)
Resources/shell-integration/cmux-bash-integration.bash
| ( | ||
| local qpwd="${pwd//\"/\\\"}" | ||
| _cmux_send "report_pwd \"${qpwd}\" --tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID" | ||
| } >/dev/null 2>&1 & disown | ||
| _cmux_send "report_pwd \"${qpwd}\" --tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID" >/dev/null 2>&1 & | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Resources/shell-integration/cmux-bash-integration.bash"
# Expectation:
# - Lines 383, 401, 415, and 1001-1004 show the new subshell pattern.
# - Lines 511-513 still show the old brace-group + disown pattern.
printf 'Changed async send sites:\n'
nl -ba "$file" | sed -n '372,418p'
printf '\nRemaining brace-group + disown send site:\n'
nl -ba "$file" | sed -n '500,516p'Repository: manaflow-ai/cmux
Length of output: 1781
🏁 Script executed:
# Fallback: attempt to list the repository details to understand availability
git ls-remote https://github.com/manaflow-ai/cmux.git HEADRepository: manaflow-ai/cmux
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Resources/shell-integration/cmux-bash-integration.bash"
printf 'Changed async send sites (expecting new subshell pattern):\n'
printf '\nLines 383:\n'
sed -n '380,386p' "$file" | cat -n
printf '\nLines 401:\n'
sed -n '398,404p' "$file" | cat -n
printf '\nLines 415:\n'
sed -n '412,418p' "$file" | cat -n
printf '\nLines 1001-1004:\n'
sed -n '999,1006p' "$file" | cat -n
printf '\n\nRemaining brace-group + disown send site (should be at 511-513):\n'
sed -n '508,516p' "$file" | cat -nRepository: manaflow-ai/cmux
Length of output: 1843
Convert remaining _cmux_emit_pr_command_hint() sender to subshell pattern to complete the fix.
This PR converts several async-send sites to the safer subshell form ( _cmux_send ... >/dev/null 2>&1 & ), but lines 511–513 in _cmux_emit_pr_command_hint() still use { _cmux_send ...; } >/dev/null 2>&1 & disown. Since _cmux_prompt_command() invokes this helper after successful gh pr ... commands, the bash job-control [n] Done ... notification leak persists. Apply the same conversion pattern to lines 511–513 to complete the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 1001 -
1004, In _cmux_emit_pr_command_hint(), replace the current async send block that
uses the braces/disown pattern (the `{ _cmux_send ...; } >/dev/null 2>&1 &
disown` code) with the subshell background pattern used elsewhere: wrap the
_cmux_send invocation in a subshell like `( _cmux_send ... >/dev/null 2>&1 & )`
so it is spawned in the background without causing bash job-control “[n] Done …”
notifications; ensure you preserve the same arguments (e.g. the `report_...`
string and `--tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID`) and quoting behavior
used in _cmux_emit_pr_command_hint().
|
Thanks for the heads up @sandipb! I noticed the CodeRabbit review also flagged a fifth Since the cmux team has their own PR for this, I'm happy to either close this PR in favor of the official one, or add the fifth fix and keep this open. Let me know what you'd prefer. I'll also check if the official PR covers all five call sites. |
|
Closing this PR since the cmux team has an official PR addressing the same issue. Thanks @sandipb for testing and the heads-up! |
|
For others who may run into this PR:
|
2 participants
Summary
Replace
{ cmd; } & disownwith( cmd & )subshell pattern in the bash integration script to prevent spurious[n] Done ...messages from appearing after every command.Problem
After every command in a cmux bash terminal, users see:
The
{ ... } & disownpattern registers a job number at creation time. When these short-lived tasks finish, bash reports completion before the next prompt — even thoughdisownwas called. This is a known bash behavior.The zsh integration already handles this correctly via
_cmux_send_bgusing&!, but the bash side lacked an equivalent fix.Fix
Changed 4 occurrences in
cmux-bash-integration.bash:_cmux_report_tty_once— tty name reporting_cmux_report_shell_activity_state— shell state sync_cmux_ports_kick— port scanner kick_cmux_prompt_command— pwd syncThe outer
( ... )subshell exits immediately. The innercmd &process becomes an orphan that bash no longer tracks, preventing any job notification. Functionality is preserved.Closes #2494
Test plan
[n] Done ...outputSummary by cubic
Stop bash job-completion messages by launching cmux background tasks in a subshell. Replace
{ cmd; } & disownwith( cmd & )so bash doesn’t track and announce jobs.[n] Done ...lines after commands in cmux bash sessions using( _cmux_send ... & )._cmux_report_tty_once,_cmux_report_shell_activity_state,_cmux_ports_kick,_cmux_prompt_command.Written for commit aabda91. Summary will update on new commits.
Summary by CodeRabbit