Repository navigation
Fix background terminal startup for unfocused workspaces - #920
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChanges introduce background surface startup logic when creating unfocused workspaces or tabs in TerminalController, and add a comprehensive test validating that the new-workspace CLI command preloads sidebar metadata without changing the currently selected workspace. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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)
tests_v2/test_cli_new_workspace_background_metadata.py (1)
70-75: Use a monotonic clock for timeout loops.At Line 71 and Line 74,
time.time()makes the timeout sensitive to wall-clock jumps;time.monotonic()is safer for polling deadlines.Proposed refactor
- deadline = time.time() + timeout + deadline = time.monotonic() + timeout @@ - while time.time() < deadline: + while time.monotonic() < deadline:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_cli_new_workspace_background_metadata.py` around lines 70 - 75, The timeout loop in _wait_for_sidebar_git_branch uses time.time() for computing deadline and checking the loop condition, which is susceptible to wall-clock changes; replace both uses of time.time() with time.monotonic() (i.e., compute deadline = time.monotonic() + timeout and loop while time.monotonic() < deadline) so the polling deadline is monotonic and robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_cli_new_workspace_background_metadata.py`:
- Around line 36-42: The test currently picks the newest file from a broad glob
(using the candidates list) which can return unrelated builds; make discovery
deterministic by first honoring an explicit CMUXTERM_CLI environment variable
(validate it is a file and executable and return it), and only if unset, gather
glob candidates, filter them to files/executables as before, then validate each
candidate by invoking the CLI (e.g., subprocess.run([candidate, "--version"] or
a known probe) and accept only those that respond with the expected
version/identifier, and finally deterministically pick the candidate by a stable
key (e.g., sorted pathname) instead of os.path.getmtime; raise cmuxError if none
pass validation.
- Around line 179-183: The cleanup block currently swallows all exceptions;
change the except to catch the specific cmuxError raised by _run_cli and log the
failure instead of silent pass. Locate the cleanup conditional that checks
created_workspace and replace the bare "except Exception: pass" with "except
cmuxError as e:" (or the proper import) and call a logger (or
pytest.fail/logging.error) to record e so cleanup failures are visible.
---
Nitpick comments:
In `@tests_v2/test_cli_new_workspace_background_metadata.py`:
- Around line 70-75: The timeout loop in _wait_for_sidebar_git_branch uses
time.time() for computing deadline and checking the loop condition, which is
susceptible to wall-clock changes; replace both uses of time.time() with
time.monotonic() (i.e., compute deadline = time.monotonic() + timeout and loop
while time.monotonic() < deadline) so the polling deadline is monotonic and
robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59a23040-1aba-49ed-b4fd-11fb643ef367
📒 Files selected for processing (2)
Sources/TerminalController.swifttests_v2/test_cli_new_workspace_background_metadata.py
| candidates = glob.glob(os.path.expanduser("~/Library/Developer/Xcode/DerivedData/**/Build/Products/Debug/cmux"), recursive=True) | ||
| candidates += glob.glob("/tmp/cmux-*/Build/Products/Debug/cmux") | ||
| candidates = [p for p in candidates if os.path.isfile(p) and os.access(p, os.X_OK)] | ||
| if not candidates: | ||
| raise cmuxError("Could not locate cmux CLI binary; set CMUXTERM_CLI") | ||
| candidates.sort(key=lambda p: os.path.getmtime(p), reverse=True) | ||
| return candidates[0] |
There was a problem hiding this comment.
Make CLI binary discovery deterministic.
At Line 36-Line 42, picking the “most recently modified” cmux from broad globs can select an unrelated build and make this regression test validate the wrong binary.
Proposed hardening
- candidates = [p for p in candidates if os.path.isfile(p) and os.access(p, os.X_OK)]
- if not candidates:
- raise cmuxError("Could not locate cmux CLI binary; set CMUXTERM_CLI")
- candidates.sort(key=lambda p: os.path.getmtime(p), reverse=True)
- return candidates[0]
+ candidates = [p for p in candidates if os.path.isfile(p) and os.access(p, os.X_OK)]
+ if not candidates:
+ raise cmuxError("Could not locate cmux CLI binary; set CMUXTERM_CLI")
+ if len(candidates) == 1:
+ return candidates[0]
+ raise cmuxError(
+ "Multiple cmux CLI binaries found; set CMUXTERM_CLI to the intended binary"
+ )🧰 Tools
🪛 Ruff (0.15.2)
[error] 37-37: Probable insecure usage of temporary file or directory: "/tmp/cmux-*/Build/Products/Debug/cmux"
(S108)
[warning] 40-40: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_cli_new_workspace_background_metadata.py` around lines 36 - 42,
The test currently picks the newest file from a broad glob (using the candidates
list) which can return unrelated builds; make discovery deterministic by first
honoring an explicit CMUXTERM_CLI environment variable (validate it is a file
and executable and return it), and only if unset, gather glob candidates, filter
them to files/executables as before, then validate each candidate by invoking
the CLI (e.g., subprocess.run([candidate, "--version"] or a known probe) and
accept only those that respond with the expected version/identifier, and finally
deterministically pick the candidate by a stable key (e.g., sorted pathname)
instead of os.path.getmtime; raise cmuxError if none pass validation.
| if created_workspace: | ||
| try: | ||
| _run_cli(cli, ["close-workspace", "--workspace", created_workspace]) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file exists and view the relevant lines with context
head -200 tests_v2/test_cli_new_workspace_background_metadata.py | tail -30Repository: manaflow-ai/cmux
Length of output: 1118
🏁 Script executed:
# Get exact line count and content around lines 179-183
wc -l tests_v2/test_cli_new_workspace_background_metadata.pyRepository: manaflow-ai/cmux
Length of output: 118
🏁 Script executed:
# View lines 170-190 to get proper context
sed -n '170,190p' tests_v2/test_cli_new_workspace_background_metadata.pyRepository: manaflow-ai/cmux
Length of output: 760
🏁 Script executed:
# Check what imports are at the top of the file
head -30 tests_v2/test_cli_new_workspace_background_metadata.pyRepository: manaflow-ai/cmux
Length of output: 745
🏁 Script executed:
# Search for cmuxError in the codebase to understand what it is
rg "cmuxError" --type pyRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
# Search for _run_cli definition to understand what exceptions it raises
rg "def _run_cli" --type py -A 10Repository: manaflow-ai/cmux
Length of output: 15013
🏁 Script executed:
# Look for similar exception handling patterns in test files
rg "except.*pass" tests_v2/ --type py -B 2 -A 1Repository: manaflow-ai/cmux
Length of output: 42
Don't silently swallow workspace cleanup failures.
The bare except Exception: pass at lines 182-183 hides cleanup failures and makes test flakes harder to diagnose. Since _run_cli raises cmuxError on failure, catch that specifically and log the error instead of silencing it.
Proposed fix
if created_workspace:
try:
_run_cli(cli, ["close-workspace", "--workspace", created_workspace])
- except Exception:
- pass
+ except cmuxError as exc:
+ print(
+ f"WARN: failed to close workspace {created_workspace!r}: {exc}",
+ file=sys.stderr,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if created_workspace: | |
| try: | |
| _run_cli(cli, ["close-workspace", "--workspace", created_workspace]) | |
| except Exception: | |
| pass | |
| if created_workspace: | |
| try: | |
| _run_cli(cli, ["close-workspace", "--workspace", created_workspace]) | |
| except cmuxError as exc: | |
| print( | |
| f"WARN: failed to close workspace {created_workspace!r}: {exc}", | |
| file=sys.stderr, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 182-183: try-except-pass detected, consider logging the exception
(S110)
[warning] 182-182: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_cli_new_workspace_background_metadata.py` around lines 179 -
183, The cleanup block currently swallows all exceptions; change the except to
catch the specific cmuxError raised by _run_cli and log the failure instead of
silent pass. Locate the cleanup conditional that checks created_workspace and
replace the bare "except Exception: pass" with "except cmuxError as e:" (or the
proper import) and call a logger (or pytest.fail/logging.error) to record e so
cleanup failures are visible.
Greptile SummaryThis PR adds calls to
The changes are minimal and defensively guarded with The code changes are safe—no crash risk from merging—and the test structure is sound. Confidence Score: 4/5
Last reviewed commit: 7efc670 |
This branch was successfully deployed
1 participant
Summary
new-workspace --cwdmetadata preloading without focus changesTesting
Summary by cubic
Fix background terminal startup for unfocused workspaces and tabs so sidebar metadata preloads without switching focus. Addresses issue #915 where new-workspace --cwd didn’t load git metadata unless focused.
Written for commit 7efc670. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests