Repository navigation
Improve heavy workspace switch latency - #4213
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughOptimizes workspace selection and handoff, centralizes pinned mount computation, makes background priming timeout instance-configurable, triggers handoff on terminal-visibility notifications, updates portal-rendering state reporting, adds related tests, and introduces a workspace-switch performance harness. ChangesWorkspace Handoff, Selection, and Priming
Sequence Diagram: high-level handoff & priming flowsequenceDiagram
participant User
participant TabManager
participant ContentView
participant BackgroundWorkspacePrimeCoordinator
participant Workspace
participant GhosttySurfaceScrollView
User->>TabManager: selectWorkspace(targetId)
alt target already selected
TabManager-->>User: return (no-op)
else different workspace
TabManager->>ContentView: activateWorkspaceCycleHotWindow()
TabManager->>TabManager: set selectedTabId
TabManager->>BackgroundWorkspacePrimeCoordinator: primeBackgroundWorkspaceIfNeeded(workspaceId)
BackgroundWorkspacePrimeCoordinator->>Workspace: waitForBackgroundWorkspacePrimeCompletion(timeoutSeconds)
Note right of BackgroundWorkspacePrimeCoordinator: defer may release mount based on reason
end
GhosttySurfaceScrollView->>ContentView: post .terminalPortalVisibilityDidChange (tabId, terminalVisibleInUI)
ContentView->>ContentView: if selected && terminalVisibleInUI then completeWorkspaceHandoffIfNeeded(reason:"terminal_visible")
ContentView->>Workspace: setPortalRenderingEnabled(enabled, reason:)
Workspace-->>ContentView: return Bool (stateChanged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 improves heavy workspace-switch latency by keeping all visited workspaces mounted (
Confidence Score: 4/5Safe to merge with one issue to resolve in the benchmark script before using it in CI. The Swift-side changes are well-tested and the logic is sound. The benchmark script has a percentile implementation that makes p95 equal to the maximum for the default 20-sample run, and the default 100 ms budget conflicts with the 279.99 ms p95 shown in the PR's own verification output. scripts/perf-workspace-switch.py — percentile implementation and default budget threshold; Sources/ContentView.swift — the always-true isPanelVisible and unbounded mount cache warrant a second look as workspace counts grow. Important Files Changed
Reviews (3): Last reviewed commit: "Speed warm workspace switches" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def read_debug_lines(self, offset: int, timeout_s: float) -> tuple[list[str], int]: | ||
| deadline = time.monotonic() + timeout_s | ||
| while time.monotonic() < deadline: | ||
| if self.debug_log_path.exists() and self.debug_log_path.stat().st_size > offset: | ||
| data = self.debug_log_path.read_text(encoding="utf-8", errors="replace") | ||
| return data[offset:].splitlines(), len(data) | ||
| time.sleep(0.01) | ||
| if self.debug_log_path.exists(): | ||
| data = self.debug_log_path.read_text(encoding="utf-8", errors="replace") | ||
| return data[offset:].splitlines(), len(data) | ||
| return [], offset |
There was a problem hiding this comment.
stat().st_size returns a byte count but len(data) returns a character count, and data[offset:] slices by character. If the debug log ever contains a non-ASCII character (e.g., a Unicode workspace name), the bookmark drifts — subsequent calls see st_size > offset as false prematurely or skip/re-parse lines, potentially producing handoff_ms: None for a run without a real missing handoff event. Open the file in binary mode and track byte offsets throughout to keep the two consistent.
| def read_debug_lines(self, offset: int, timeout_s: float) -> tuple[list[str], int]: | |
| deadline = time.monotonic() + timeout_s | |
| while time.monotonic() < deadline: | |
| if self.debug_log_path.exists() and self.debug_log_path.stat().st_size > offset: | |
| data = self.debug_log_path.read_text(encoding="utf-8", errors="replace") | |
| return data[offset:].splitlines(), len(data) | |
| time.sleep(0.01) | |
| if self.debug_log_path.exists(): | |
| data = self.debug_log_path.read_text(encoding="utf-8", errors="replace") | |
| return data[offset:].splitlines(), len(data) | |
| return [], offset | |
| def read_debug_lines(self, offset: int, timeout_s: float) -> tuple[list[str], int]: | |
| deadline = time.monotonic() + timeout_s | |
| while time.monotonic() < deadline: | |
| if self.debug_log_path.exists() and self.debug_log_path.stat().st_size > offset: | |
| raw = self.debug_log_path.read_bytes() | |
| chunk = raw[offset:].decode("utf-8", errors="replace") | |
| return chunk.splitlines(), len(raw) | |
| time.sleep(0.01) | |
| if self.debug_log_path.exists(): | |
| raw = self.debug_log_path.read_bytes() | |
| chunk = raw[offset:].decode("utf-8", errors="replace") | |
| return chunk.splitlines(), len(raw) | |
| return [], offset |
| func selectWorkspace(_ workspace: Workspace) { | ||
| guard selectedTabId != workspace.id else { | ||
| #if DEBUG | ||
| debugPrimeWorkspaceSwitchTrigger("select", to: workspace.id) | ||
| #endif | ||
| return | ||
| } | ||
| #if DEBUG | ||
| debugPrimeWorkspaceSwitchTrigger("select", to: workspace.id) | ||
| #endif | ||
| activateWorkspaceCycleHotWindow() | ||
| selectedTabId = workspace.id | ||
| } |
There was a problem hiding this comment.
debugPrimeWorkspaceSwitchTrigger call appears in both the early-return branch and the fall-through branch with identical arguments. The function already guards on selectedTabId != target internally — calling it once, before the guard, produces the same effect in both paths and removes the duplication. As written, a future reader has to know the internal logic of debugPrimeWorkspaceSwitchTrigger to confirm the two calls aren't accidentally doing different things.
| func selectWorkspace(_ workspace: Workspace) { | |
| guard selectedTabId != workspace.id else { | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select", to: workspace.id) | |
| #endif | |
| return | |
| } | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select", to: workspace.id) | |
| #endif | |
| activateWorkspaceCycleHotWindow() | |
| selectedTabId = workspace.id | |
| } | |
| func selectWorkspace(_ workspace: Workspace) { | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select", to: workspace.id) | |
| #endif | |
| guard selectedTabId != workspace.id else { return } | |
| activateWorkspaceCycleHotWindow() | |
| selectedTabId = workspace.id | |
| } |
| func selectTab(at index: Int) { | ||
| guard index >= 0 && index < tabs.count else { return } | ||
| let tabId = tabs[index].id | ||
| guard selectedTabId != tabId else { | ||
| #if DEBUG | ||
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabId) | ||
| #endif | ||
| return | ||
| } | ||
| #if DEBUG | ||
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabs[index].id) | ||
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabId) | ||
| #endif | ||
| selectedTabId = tabs[index].id | ||
| activateWorkspaceCycleHotWindow() | ||
| selectedTabId = tabId | ||
| } |
There was a problem hiding this comment.
selectWorkspace. Since debugPrimeWorkspaceSwitchTrigger guards internally on selectedTabId != target, hoisting the call above the guard covers both paths with one block.
| func selectTab(at index: Int) { | |
| guard index >= 0 && index < tabs.count else { return } | |
| let tabId = tabs[index].id | |
| guard selectedTabId != tabId else { | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabId) | |
| #endif | |
| return | |
| } | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabs[index].id) | |
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabId) | |
| #endif | |
| selectedTabId = tabs[index].id | |
| activateWorkspaceCycleHotWindow() | |
| selectedTabId = tabId | |
| } | |
| func selectTab(at index: Int) { | |
| guard index >= 0 && index < tabs.count else { return } | |
| let tabId = tabs[index].id | |
| #if DEBUG | |
| debugPrimeWorkspaceSwitchTrigger("select_index", to: tabId) | |
| #endif | |
| guard selectedTabId != tabId else { return } | |
| activateWorkspaceCycleHotWindow() | |
| selectedTabId = tabId | |
| } |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/perf-workspace-switch.py`:
- Around line 219-224: The current raise of PerfFailure in
scripts/perf-workspace-switch.py exposes raw CLI stdout/stderr (when
proc.returncode != 0), which must be sanitized; change the PerfFailure message
to a short, generic error that includes the failing command (from args) and the
return code only, and instruct operators to consult local logs for full output
instead of embedding proc.stdout/proc.stderr in the exception; ensure related
call sites like run_cli and other raised PerfFailure occurrences (e.g., the
blocks around lines 516-517 and 580-588) follow the same pattern and that the
full proc output is written to secure local logs (not the exception) for
debugging.
- Around line 455-465: Detect when the measured list is empty and fail the
benchmark immediately instead of computing summaries: in the block that builds
self.result["measurements"]["workspace_switch"] (where measured, cli_values,
handoff_values, async_values and summary(...) are used), add a guard like if not
measured: raise RuntimeError("no workspace switch measurements collected") (or
set an explicit failure marker used by apply_budgets()), so a misconfigured run
(e.g. --measure-passes 0) cannot silently pass; ensure the check runs before
calling summary() for cli_roundtrip/handoff/async_done and before computing
missing_handoff_samples.
- Around line 109-113: clean_persisted_state only deletes tag-scoped session
files but the launched app/CLI can still inherit ambient CMUX_* values; ensure
full isolation by (1) when launching the app and any CLI subprocesses, inject
the derived tag-scoped environment variables CMUX_TAG and CMUX_BUNDLE_ID (use
the existing bundle_id/tag_id values) into the subprocess env and (2)
scrub/unset any other ambient CMUX_* keys from that subprocess env so it cannot
attach to an existing session; also extend clean_persisted_state to remove both
the tag-scoped session files and any non-tagged session files that could be
reused, and apply these env-injection/scrubbing changes in the code paths that
start the app/CLI (refer to clean_persisted_state, tag_id, and bundle_id to
locate the logic).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4662e844-5fe7-417a-83c2-10c100d325c2
📒 Files selected for processing (9)
Sources/BackgroundWorkspacePrimeCoordinator.swiftSources/ContentView.swiftSources/GhosttyTerminalAppearance.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceUnitTests.swiftscripts/perf-workspace-switch.py
| if check and proc.returncode != 0: | ||
| raise PerfFailure( | ||
| "cmux command failed: " | ||
| + " ".join(args) | ||
| + f"\nstdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" | ||
| ) |
There was a problem hiding this comment.
Stop surfacing raw CLI stdout/stderr in failures.
The exception text from run_cli() is copied into result["failures"], written to JUnit/JSON, printed, and then re-thrown. That makes any raw CLI error body user-visible, which is exactly the kind of upstream/internal leakage the review rules prohibit. Emit a short sanitized failure here and point operators to the local logs for full details instead.
Suggested fix
if check and proc.returncode != 0:
raise PerfFailure(
- "cmux command failed: "
- + " ".join(args)
- + f"\nstdout:\n{proc.stdout}\nstderr:\n{proc.stderr}"
+ f"cmux command failed: {' '.join(args)}. "
+ f"See {self.stdout_path} and {self.debug_log_path} for details."
)As per coding guidelines, "user-facing errors, alerts, command output, API error bodies, or recovery copy must not expose upstream vendor names, internal provider names, provider-specific flags, templates, snapshots, manifests, environment variables, database or migration details, raw upstream messages, billing item ids, billing customer ids, unrelated team ids, credentials, tokens, headers, private keys, refresh tokens, session ids, or unredacted payload dumps".
Also applies to: 516-517, 580-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/perf-workspace-switch.py` around lines 219 - 224, The current raise
of PerfFailure in scripts/perf-workspace-switch.py exposes raw CLI stdout/stderr
(when proc.returncode != 0), which must be sanitized; change the PerfFailure
message to a short, generic error that includes the failing command (from args)
and the return code only, and instruct operators to consult local logs for full
output instead of embedding proc.stdout/proc.stderr in the exception; ensure
related call sites like run_cli and other raised PerfFailure occurrences (e.g.,
the blocks around lines 516-517 and 580-588) follow the same pattern and that
the full proc output is written to secure local logs (not the exception) for
debugging.
| measured = [s for s in samples if s["phase"] == "measure"] | ||
| cli_values = [float(s["cli_roundtrip_ms"]) for s in measured] | ||
| handoff_values = [float(s["handoff_ms"]) for s in measured if s["handoff_ms"] is not None] | ||
| async_values = [float(s["async_done_ms"]) for s in measured if s["async_done_ms"] is not None] | ||
| self.result["measurements"]["workspace_switch"] = { | ||
| "samples": measured, | ||
| "cli_roundtrip": summary(cli_values), | ||
| "handoff": summary(handoff_values), | ||
| "async_done": summary(async_values), | ||
| "missing_handoff_samples": len(measured) - len(handoff_values), | ||
| } |
There was a problem hiding this comment.
Fail the benchmark when no measured switches were collected.
An empty measured set currently becomes summary([]) == 0 everywhere, and apply_budgets() will report a pass because missing_handoff_samples is also 0. A misconfigured run like --measure-passes 0 would therefore go green with no benchmark data.
Suggested fix
measured = [s for s in samples if s["phase"] == "measure"]
+ if not measured:
+ raise PerfFailure("workspace switch benchmark collected no measured samples")
cli_values = [float(s["cli_roundtrip_ms"]) for s in measured]
handoff_values = [float(s["handoff_ms"]) for s in measured if s["handoff_ms"] is not None]
async_values = [float(s["async_done_ms"]) for s in measured if s["async_done_ms"] is not None]
self.result["measurements"]["workspace_switch"] = {
"samples": measured,Also applies to: 467-486
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/perf-workspace-switch.py` around lines 455 - 465, Detect when the
measured list is empty and fail the benchmark immediately instead of computing
summaries: in the block that builds
self.result["measurements"]["workspace_switch"] (where measured, cli_values,
handoff_values, async_values and summary(...) are used), add a guard like if not
measured: raise RuntimeError("no workspace switch measurements collected") (or
set an explicit failure marker used by apply_budgets()), so a misconfigured run
(e.g. --measure-passes 0) cannot silently pass; ensure the check runs before
calling summary() for cli_roundtrip/handoff/async_done and before computing
missing_handoff_samples.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 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="scripts/perf-workspace-switch.py">
<violation number="1" location="scripts/perf-workspace-switch.py:41">
P2: The percentile index formula is off by one for p95 and often returns the max sample instead of the 95th percentile, skewing reported latency and budget checks.</violation>
<violation number="2" location="scripts/perf-workspace-switch.py:389">
P2: Use byte-based reads and offsets in `read_debug_lines`. Mixing `st_size` byte offsets with decoded string indices can desynchronize log scanning on non-ASCII output and miss handoff completion events.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| if not sorted_values: | ||
| return 0.0 | ||
| clamped = min(max(pct, 0.0), 1.0) | ||
| index = int(((len(sorted_values) - 1) * clamped) + 0.999999) |
There was a problem hiding this comment.
P2: The percentile index formula is off by one for p95 and often returns the max sample instead of the 95th percentile, skewing reported latency and budget checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/perf-workspace-switch.py, line 41:
<comment>The percentile index formula is off by one for p95 and often returns the max sample instead of the 95th percentile, skewing reported latency and budget checks.</comment>
<file context>
@@ -0,0 +1,606 @@
+ if not sorted_values:
+ return 0.0
+ clamped = min(max(pct, 0.0), 1.0)
+ index = int(((len(sorted_values) - 1) * clamped) + 0.999999)
+ return sorted_values[min(len(sorted_values) - 1, max(0, index))]
+
</file context>
| index = int(((len(sorted_values) - 1) * clamped) + 0.999999) | |
| index = max(0, min(len(sorted_values) - 1, int((len(sorted_values) * clamped) + 0.999999) - 1)) |
| while time.monotonic() < deadline: | ||
| if self.debug_log_path.exists() and self.debug_log_path.stat().st_size > offset: | ||
| data = self.debug_log_path.read_text(encoding="utf-8", errors="replace") | ||
| return data[offset:].splitlines(), len(data) |
There was a problem hiding this comment.
P2: Use byte-based reads and offsets in read_debug_lines. Mixing st_size byte offsets with decoded string indices can desynchronize log scanning on non-ASCII output and miss handoff completion events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/perf-workspace-switch.py, line 389:
<comment>Use byte-based reads and offsets in `read_debug_lines`. Mixing `st_size` byte offsets with decoded string indices can desynchronize log scanning on non-ASCII output and miss handoff completion events.</comment>
<file context>
@@ -0,0 +1,606 @@
+ while time.monotonic() < deadline:
+ if self.debug_log_path.exists() and self.debug_log_path.stat().st_size > offset:
+ data = self.debug_log_path.read_text(encoding="utf-8", errors="replace")
+ return data[offset:].splitlines(), len(data)
+ time.sleep(0.01)
+ if self.debug_log_path.exists():
</file context>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (3)
scripts/perf-workspace-switch.py (3)
219-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize CLI failure messages to avoid exposing raw output.
The exception includes raw
proc.stdoutandproc.stderr, which could leak environment variables, internal paths, or upstream error messages. Per coding guidelines, user-facing errors must not expose raw upstream messages.Proposed fix
if check and proc.returncode != 0: raise PerfFailure( - "cmux command failed: " - + " ".join(args) - + f"\nstdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + f"cmux command failed (exit {proc.returncode}): {' '.join(args)}. " + f"See {self.debug_log_path} for details." )As per coding guidelines, "user-facing errors, alerts, command output, API error bodies, or recovery copy must not expose upstream vendor names, internal provider names, ... raw upstream messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/perf-workspace-switch.py` around lines 219 - 224, The PerfFailure raised in the block that checks "check and proc.returncode != 0" currently includes raw proc.stdout and proc.stderr which can leak sensitive/internal info; change the error to include only a sanitized summary (e.g., a short, redacted message or first N characters with ellipsis, or a standardized failure token) and include the full proc output only in a debug/internal log; update the raise PerfFailure(...) call (referencing PerfFailure, proc.returncode, args, proc.stdout, proc.stderr) to replace raw outputs with the sanitized summary and ensure any detailed output is logged via a non-user-facing logger or attached to diagnostics only.
455-465:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against empty measurement set to prevent silent pass.
If
measuredis empty (e.g.,--measure-passes 0or fixture creation fails silently), all summaries return zeros andapply_budgets()reports a pass with no actual benchmark data.Proposed fix
measured = [s for s in samples if s["phase"] == "measure"] + if not measured: + raise PerfFailure("workspace switch benchmark collected no measured samples") cli_values = [float(s["cli_roundtrip_ms"]) for s in measured]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/perf-workspace-switch.py` around lines 455 - 465, The code currently computes summaries from measured (the list comprehension filtering samples by phase) and writes zero summaries when measured is empty; update the workspace switch measurement block to explicitly handle an empty measured set: after computing measured (variable name measured) check if not measured and then raise a clear exception or set self.result["measurements"]["workspace_switch"] to indicate no data (and ensure apply_budgets sees this as a failure) instead of calling summary(...) on empty lists; reference the summary(...) calls and the keys "cli_roundtrip", "handoff", "async_done", and "missing_handoff_samples" so you modify that block to short-circuit and produce an error/result flag when measured is empty.
153-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCLI environment lacks ambient variable scrubbing.
app_env()correctly scrubs ambientCMUX_*variables (lines 124-141), butcli_env()only adds a few variables without first removing inherited ones. If this script runs from within an existing cmux shell, CLI commands may inadvertently connect to the wrong session.Proposed fix
def cli_env(self) -> dict[str, str]: env = os.environ.copy() + for key in ( + "CMUX_SOCKET", + "CMUX_SOCKET_PATH", + "CMUX_SOCKET_MODE", + "CMUX_TAB_ID", + "CMUX_PANEL_ID", + "CMUX_SURFACE_ID", + "CMUX_WORKSPACE_ID", + "CMUXD_UNIX_PATH", + "CMUX_TAG", + "CMUX_PORT", + "CMUX_PORT_END", + "CMUX_PORT_RANGE", + "CMUX_DEBUG_LOG", + "CMUX_BUNDLE_ID", + "CMUX_UI_TEST_MODE", + ): + env.pop(key, None) env["CMUX_SOCKET"] = str(self.socket_path) env["CMUX_SOCKET_PATH"] = str(self.socket_path) + env["CMUXD_UNIX_PATH"] = str(self.cmuxd_socket_path) env["CMUXTERM_CLI_RESPONSE_TIMEOUT_SEC"] = "30" return env🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/perf-workspace-switch.py` around lines 153 - 158, The cli_env() function copies the current environment but fails to scrub ambient CMUX_* variables first, which can cause CLI commands to bind to an existing cmux session; update cli_env() to mirror app_env() by creating env = os.environ.copy(), removing any inherited keys that start with "CMUX_" (or the same explicit list used in app_env()), then set/override env["CMUX_SOCKET"], env["CMUX_SOCKET_PATH"], and env["CMUXTERM_CLI_RESPONSE_TIMEOUT_SEC"]="30" before returning env so the CLI uses only the intended socket and settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/perf-workspace-switch.py`:
- Around line 219-224: The PerfFailure raised in the block that checks "check
and proc.returncode != 0" currently includes raw proc.stdout and proc.stderr
which can leak sensitive/internal info; change the error to include only a
sanitized summary (e.g., a short, redacted message or first N characters with
ellipsis, or a standardized failure token) and include the full proc output only
in a debug/internal log; update the raise PerfFailure(...) call (referencing
PerfFailure, proc.returncode, args, proc.stdout, proc.stderr) to replace raw
outputs with the sanitized summary and ensure any detailed output is logged via
a non-user-facing logger or attached to diagnostics only.
- Around line 455-465: The code currently computes summaries from measured (the
list comprehension filtering samples by phase) and writes zero summaries when
measured is empty; update the workspace switch measurement block to explicitly
handle an empty measured set: after computing measured (variable name measured)
check if not measured and then raise a clear exception or set
self.result["measurements"]["workspace_switch"] to indicate no data (and ensure
apply_budgets sees this as a failure) instead of calling summary(...) on empty
lists; reference the summary(...) calls and the keys "cli_roundtrip", "handoff",
"async_done", and "missing_handoff_samples" so you modify that block to
short-circuit and produce an error/result flag when measured is empty.
- Around line 153-158: The cli_env() function copies the current environment but
fails to scrub ambient CMUX_* variables first, which can cause CLI commands to
bind to an existing cmux session; update cli_env() to mirror app_env() by
creating env = os.environ.copy(), removing any inherited keys that start with
"CMUX_" (or the same explicit list used in app_env()), then set/override
env["CMUX_SOCKET"], env["CMUX_SOCKET_PATH"], and
env["CMUXTERM_CLI_RESPONSE_TIMEOUT_SEC"]="30" before returning env so the CLI
uses only the intended socket and settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e44eb5e-2a8b-4d43-95c8-b6bcc19b73b0
📒 Files selected for processing (2)
Sources/ContentView.swiftscripts/perf-workspace-switch.py
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="scripts/perf-workspace-switch.py">
<violation number="1" location="scripts/perf-workspace-switch.py:568">
P2: The new default handoff budget (`100ms`) is too strict for default runs and can cause the benchmark to fail by default.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
|
|
||
| parser.add_argument("--launch-timeout", type=float, default=45) | ||
| parser.add_argument("--switch-log-timeout", type=float, default=5) | ||
| parser.add_argument("--budget-handoff-p95-ms", type=float, default=100) |
There was a problem hiding this comment.
P2: The new default handoff budget (100ms) is too strict for default runs and can cause the benchmark to fail by default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/perf-workspace-switch.py, line 568:
<comment>The new default handoff budget (`100ms`) is too strict for default runs and can cause the benchmark to fail by default.</comment>
<file context>
@@ -565,8 +565,8 @@ def parse_args() -> argparse.Namespace:
parser.add_argument("--switch-log-timeout", type=float, default=5)
- parser.add_argument("--budget-handoff-p95-ms", type=float, default=300)
- parser.add_argument("--budget-cli-roundtrip-p95-ms", type=float, default=450)
+ parser.add_argument("--budget-handoff-p95-ms", type=float, default=100)
+ parser.add_argument("--budget-cli-roundtrip-p95-ms", type=float, default=0)
return parser.parse_args()
</file context>
| parser.add_argument("--budget-handoff-p95-ms", type=float, default=100) | |
| parser.add_argument("--budget-handoff-p95-ms", type=float, default=300) |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 323c736. Configure here.
| ) | ||
| #endif | ||
| hostView.addSubview(containerView, positioned: .above, relativeTo: nil) | ||
| } |
There was a problem hiding this comment.
Browser portal missing becameVisible reparenting check
Low Severity
BrowserWindowPortal.updateEntryVisibility only reparents the container view to the front when priorityIncreased is true, but TerminalWindowPortal.updateEntryVisibility reparents on becameVisible || priorityIncreased. This means a browser portal entry that transitions from invisible to visible without a z-priority increase won't be brought to the top of the subview hierarchy, unlike its terminal counterpart. While current callers generally change both visibility and priority together, this inconsistency could cause stale browser z-ordering in future code paths.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 323c736. Configure here.
| let didPrioritizeSelected = selected.flatMap { selectedId in | ||
| tabManager.tabs.first(where: { $0.id == selectedId })? | ||
| .prioritizePortalViewsForCurrentRenderedLayout(zPriority: 2, reason: "workspaceHandoff.selected") | ||
| } ?? false |
There was a problem hiding this comment.
Debug-only variable usage causes Release build warnings
Low Severity
didLowerRetiring and didPrioritizeSelected are computed unconditionally but only read inside the #if DEBUG block. In Release builds, these let bindings are never read, which generates "immutable value was never used" compiler warnings. The side-effect calls to prioritizePortalViewsForCurrentRenderedLayout are needed in all configurations, but the return value capture creates unnecessary noise.
Reviewed by Cursor Bugbot for commit 323c736. Configure here.
| func selectLastTab() { | ||
| guard let lastTab = tabs.last else { return } | ||
| guard selectedTabId != lastTab.id else { return } | ||
| activateWorkspaceCycleHotWindow() |
There was a problem hiding this comment.
selectLastTab missing debug workspace switch tracking
Low Severity
selectLastTab now calls activateWorkspaceCycleHotWindow() (consistent with selectWorkspace and selectTab(at:)), but unlike those methods it omits the debugPrimeWorkspaceSwitchTrigger call. This means workspace-switch debug tracking and benchmark instrumentation won't capture switches triggered via selectLastTab, making the performance telemetry inconsistent across selection paths.
Reviewed by Cursor Bugbot for commit 323c736. Configure here.
| raise | ||
|
|
||
| if args.output: | ||
| output = pathlib.Path(args.output) | ||
| output.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
percentile at p95 equals max for 20 samples; default budget inconsistent with reported results
The percentile function adds 0.999999 before truncating, which rounds every fractional index up to the next integer. For the default run (6 workspaces, 2 measure passes = 20 measured switches): int((20-1) * 0.95 + 0.999999) = int(19.05) = 19 → sorted_values[19] = the maximum value. So p95_ms is always the worst sample, not the 95th-percentile sample.
Separately, the PR description reports a verified handoff p95 of 279.99 ms, but the default --budget-handoff-p95-ms is 100 ms. Running the script as documented (--measure-passes 2 --workspace-count 6) would produce a budget failure on the very benchmark the PR claims passes. The verification must have used a custom budget that isn't shown in the command, leaving the default misleading for future users.
There was a problem hiding this comment.
1 issue found across 7 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/BrowserWindowPortal.swift">
<violation number="1" location="Sources/BrowserWindowPortal.swift:2842">
P3: Include hidden→visible transitions in the reparent condition, not just z-priority increases. Otherwise a browser portal entry can become visible but remain behind older siblings with stale z-order.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| entriesByWebViewId[webViewId] = entry | ||
| let priorityIncreased = zPriority > previousZPriority | ||
| if visibleInUI, | ||
| priorityIncreased, |
There was a problem hiding this comment.
P3: Include hidden→visible transitions in the reparent condition, not just z-priority increases. Otherwise a browser portal entry can become visible but remain behind older siblings with stale z-order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserWindowPortal.swift, line 2842:
<comment>Include hidden→visible transitions in the reparent condition, not just z-priority increases. Otherwise a browser portal entry can become visible but remain behind older siblings with stale z-order.</comment>
<file context>
@@ -2823,9 +2833,24 @@ final class WindowBrowserPortal: NSObject {
entriesByWebViewId[webViewId] = entry
+ let priorityIncreased = zPriority > previousZPriority
+ if visibleInUI,
+ priorityIncreased,
+ let containerView = entry.containerView,
+ containerView.superview === hostView,
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary:
Cloud benchmark:
scripts/perf-workspace-switch.py --tag cwarm3 --workspace-count 6 --panes-per-workspace 5 --surfaces-per-pane 2 --browser-panes-per-workspace 2 --terminal-scrollback-lines 200 --browser-dom-rows 240 --warmup-passes 1 --measure-passes 2 --output /tmp/cmux-switch-cwarm3.jsonCloud verification:
CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination "platform=macOS,arch=arm64" -derivedDataPath /tmp/cmux-cwarm3-tests-skipzig -only-testing:cmuxTests/WorkspaceMountPolicyTests -only-testing:cmuxTests/TabManagerWorkspaceSelectionPerfTests -only-testing:cmuxTests/WorkspacePortalRenderingTests testCMUX_SKIP_ZIG_BUILD=1 xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination "platform=macOS,arch=arm64" -derivedDataPath /tmp/cmux-cwarm3-tests-skipzig -only-testing:cmuxTests/WorkspaceContentViewVisibilityTests/testNonSelectedNonRetiringWorkspaceKeepsPanelMountedButTransparent test