Repository navigation
Add debug.sidebar.simulate_drag for headless drag profiling - #4771
Merged
Merged
Conversation
Adds a DEBUG-only V2 socket method that drives SidebarDragState mutations (draggedTabId + dropIndicator) across N steps over a configurable duration, plus a `cmux simulate-sidebar-drag` CLI wrapper. Intended as the deterministic workload generator for the profile-pr skill in cmuxterm-hq, which wraps it with xctrace to compare a PR's HEAD vs its merge base. Why mutation-only instead of HID synthesis: the architectural claim under test on perf-sensitive sidebar PRs is that @observable per-property tracking plus TabItemView Equatable conformance contains drag invalidation to the dragged row and its indicator overlays. That whole hot path lives downstream of dragState mutations; driving the mutations directly captures the cost we care about, while sidestepping flaky NSEvent synthesis. Pieces: - SidebarDragStateRegistry (DEBUG-only): per-windowId map populated on VerticalTabsSidebar.onAppear / cleared on .onDisappear, so the socket handler can resolve a live dragState without plumbing through AppDelegate. - v2DebugSidebarSimulateDrag in TerminalController: validates window/from/to UUIDs, computes the step path on the main actor, then ticks dragState with Thread.sleep between updates (test-only scaffolding, gated behind #if DEBUG per CLAUDE.md's sleep rule). Never commits a reorder. - `cmux simulate-sidebar-drag` CLI subcommand + help text. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DEBUG-only SidebarDragState registry, a TerminalController v2 handler that simulates sidebar drag by mutating drag state across timed steps, and a CLI ChangesSidebar drag simulation for debugging
Sequence DiagramsequenceDiagram
participant CLI as CLI/cmux
participant SocketWorker as TerminalController (socket worker)
participant Registry as SidebarDragStateRegistry
participant DragState as SidebarDragState (main)
CLI->>SocketWorker: sendV2 method="debug.sidebar.simulate_drag" params
SocketWorker->>Registry: state(forWindowId:)
Registry-->>SocketWorker: SidebarDragState reference
SocketWorker->>DragState: v2MainSync set draggedTabId / dropIndicator (step N)
SocketWorker->>SocketWorker: Thread.sleep(duration per step)
SocketWorker->>DragState: v2MainSync update draggedTabId / dropIndicator (next step)
SocketWorker->>DragState: v2MainSync clear drag state (completion)
SocketWorker-->>CLI: return .ok payload with path/timing/steps
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 (3 errors, 1 warning)
✅ Passed checks (13 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — this is a well-scoped DEBUG-only profiling tool with no release-path behavioral changes. All three issues flagged in the prior review round are correctly addressed. The simulation runs on the socket worker, No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant SW as Socket Worker
participant MA as Main Actor (v2MainSync)
participant Reg as SidebarDragStateRegistry
participant DS as SidebarDragState
CLI->>SW: debug.sidebar.simulate_drag(params)
SW->>MA: v2MainSync – plan phase
MA->>Reg: state(forWindowId:) – existence check
MA->>MA: resolve tabIds, fromIndex, toIndex
MA-->>SW: PlanResult.ok(...)
SW->>MA: v2MainSync – start drag
MA->>Reg: state(forWindowId:)
MA->>DS: "isSimulated = true"
MA->>DS: "draggedTabId = fromTabId"
MA-->>SW: "startedOK = true"
loop N steps (Thread.sleep between)
SW->>MA: v2MainSync – tick
MA->>Reg: state(forWindowId:)
MA->>DS: "dropIndicator = SidebarDropIndicator(...)"
MA-->>SW: tickOK
SW->>SW: Thread.sleep(stepIntervalMs)
end
SW->>MA: v2MainSync – teardown
MA->>DS: "draggedTabId = nil"
MA->>DS: "dropIndicator = nil"
MA->>DS: "isSimulated = false"
SW-->>CLI: "{steps, duration_ms, edge, path, ...}"
Reviews (7): Last reviewed commit: "Defensive: reset isSimulated in .onAppea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| dragState.dropIndicator = SidebarDropIndicator(tabId: targetTabId, edge: edge) | ||
| } | ||
| if stepIntervalMs > 0 { | ||
| Thread.sleep(forTimeInterval: TimeInterval(stepIntervalMs) / 1000.0) |
Contributor
There was a problem hiding this comment.
Thread.sleep runs on the main actor, blocking the run loop
debug.sidebar.simulate_drag is absent from socketWorkerV2Methods, so executionPolicy returns .mainActor for it. The method is therefore dispatched through the .mainActor path in processCommand, meaning the entire simulation loop — including Thread.sleep — runs on the main thread. v2MainSync detects Thread.isMainThread == true and takes the assumeIsolated fast path (not DispatchQueue.main.sync), so there is no background hop. The main run loop is blocked for the full simulation duration, which prevents SwiftUI from scheduling frame renders between drag mutations. The profiling capture will see one burst of state changes at teardown rather than the continuous per-step invalidation the tool is designed to drive.
Fix: add "debug.sidebar.simulate_drag" to socketWorkerV2Methods so it receives .socketWorker dispatch, move the implementation into the nonisolated socketWorkerV2Response switch (the method already uses v2MainSync for every @MainActor operation, so the conversion is straightforward), and mark v2DebugSidebarSimulateDrag as nonisolated. With that change the sleep blocks only the socket-worker thread and main is free to render frames between each step — which is exactly what xctrace needs to see.
Collaborator
Author
There was a problem hiding this comment.
Fixed in 70181a2 — added the method to socketWorkerV2Methods and registered the case in the worker dispatch switch, marked the handler nonisolated, and pushed all parameter resolution + dragState reads/writes through v2MainSync. Thread.sleep now runs on the worker thread; the main actor stays free to render between ticks.
— Claude Code
| } | ||
| #endif | ||
|
|
||
| private func v2DebugReadTerminalText(params: [String: Any]) -> V2CallResult { |
Contributor
There was a problem hiding this comment.
system.capabilities
debug.terminal.simulate_file_drop is advertised via methods.append(...) inside a #if DEBUG block (line 4022), but debug.sidebar.simulate_drag has no matching entry. The profile-pr skill and any client that introspects capabilities before calling the method will not discover it, making the integration more fragile — a capability check returning false could silently skip the profiling step.
Collaborator
Author
There was a problem hiding this comment.
Fixed in 70181a2 — debug.sidebar.simulate_drag now advertised in system.capabilities under the existing DEBUG-only methods.append block, matching debug.terminal.simulate_file_drop.
— Claude Code
![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
🤖 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 `@CLI/cmux.swift`:
- Around line 6013-6047: The new user-facing strings (the CLIError messages, the
duration/steps validation messages, and the summary passed to printV2Payload)
must be localized instead of hard-coded; replace each literal string in this
block (e.g., the messages thrown where you call CLIError, the "--duration-ms
must be a positive integer"/"--steps must be a positive integer" texts, and the
summary "OK steps=...") with String(localized:defaultValue:) (or the project’s
localization helper) and add corresponding entries to the app’s string catalog
for all supported locales; ensure you update the same pattern for other
occurrences in this file (including the normalizeWorkspaceHandle/optionValue
call sites that produce CLI errors) so every CLI-facing message uses the
localization API and has matching catalog keys/translations.
In `@Sources/TerminalController.swift`:
- Around line 15599-15604: The handler currently coerces invalid duration_ms and
steps instead of rejecting them; change validation around the v2Int calls so
that if params contains "duration_ms" or "steps" but v2Int returns nil or a
non-positive value you return .err(code: "invalid_params", message: "...")
rather than falling back to defaults or forcing steps to 1. Concretely, after
calling let durationMs = v2Int(params, "duration_ms") and let requestedSteps =
v2Int(params, "steps"), check if params.keys.contains("duration_ms") and
durationMs == nil or durationMs! <= 0 and likewise for "steps" (nil or <= 0) and
return the invalid_params error; otherwise apply the existing defaults only when
the key is absent. Ensure the same change is applied at the other occurrence
referenced (around requestedSteps usage).
🪄 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: a887490c-96dd-497a-8c44-b28ef239a231
📒 Files selected for processing (3)
CLI/cmux.swiftSources/ContentView.swiftSources/TerminalController.swift
Comment on lines
+6013
to
+6047
| throw CLIError(message: "simulate-sidebar-drag requires --window <id|ref|index>") | ||
| } | ||
| guard let fromRaw = optionValue(commandArgs, name: "--from") else { | ||
| throw CLIError(message: "simulate-sidebar-drag requires --from <workspace id|ref|index>") | ||
| } | ||
| guard let toRaw = optionValue(commandArgs, name: "--to") else { | ||
| throw CLIError(message: "simulate-sidebar-drag requires --to <workspace id|ref|index>") | ||
| } | ||
| let fromHandle = try normalizeWorkspaceHandle(fromRaw, client: client, windowHandle: windowHandle) | ||
| let toHandle = try normalizeWorkspaceHandle(toRaw, client: client, windowHandle: windowHandle) | ||
| guard let fromHandle, let toHandle else { | ||
| throw CLIError(message: "simulate-sidebar-drag could not resolve --from / --to to workspace ids") | ||
| } | ||
|
|
||
| var params: [String: Any] = [ | ||
| "window_id": windowHandle, | ||
| "from_tab_id": fromHandle, | ||
| "to_tab_id": toHandle | ||
| ] | ||
| if let durationRaw = optionValue(commandArgs, name: "--duration-ms") { | ||
| guard let duration = Int(durationRaw), duration > 0 else { | ||
| throw CLIError(message: "--duration-ms must be a positive integer") | ||
| } | ||
| params["duration_ms"] = duration | ||
| } | ||
| if let stepsRaw = optionValue(commandArgs, name: "--steps") { | ||
| guard let steps = Int(stepsRaw), steps > 0 else { | ||
| throw CLIError(message: "--steps must be a positive integer") | ||
| } | ||
| params["steps"] = steps | ||
| } | ||
|
|
||
| let payload = try client.sendV2(method: "debug.sidebar.simulate_drag", params: params) | ||
| let summary = "OK steps=\(payload["steps"] ?? "?") duration_ms=\(payload["duration_ms"] ?? "?") edge=\(payload["edge"] ?? "?")" | ||
| printV2Payload(payload, jsonOutput: jsonOutput, idFormat: idFormat, fallbackText: summary) |
There was a problem hiding this comment.
Localize newly introduced CLI-facing strings.
These new error/help/summary strings are hard-coded English. Route them through the project’s localization API and add matching catalog entries for all supported locales.
As per coding guidelines: “For production changes, fail when the diff violates .github/review-bot-rules/full-internationalization.md: user-facing Swift text must use String(localized:defaultValue:) or an equivalent localized API with a matching translated string-catalog entry … for every locale.”
Also applies to: 12112-12131
🤖 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 `@CLI/cmux.swift` around lines 6013 - 6047, The new user-facing strings (the
CLIError messages, the duration/steps validation messages, and the summary
passed to printV2Payload) must be localized instead of hard-coded; replace each
literal string in this block (e.g., the messages thrown where you call CLIError,
the "--duration-ms must be a positive integer"/"--steps must be a positive
integer" texts, and the summary "OK steps=...") with
String(localized:defaultValue:) (or the project’s localization helper) and add
corresponding entries to the app’s string catalog for all supported locales;
ensure you update the same pattern for other occurrences in this file (including
the normalizeWorkspaceHandle/optionValue call sites that produce CLI errors) so
every CLI-facing message uses the localization API and has matching catalog
keys/translations.
Collaborator
Author
There was a problem hiding this comment.
Skipping — these are CLI dev-tool diagnostics (CLIError messages and per-command help text), not in-app UI strings. Existing CLI subcommands like reorder-workspace use the same hard-coded English pattern (CLI/cmux.swift:5885,5903). The CLAUDE.md localization rule targets user-facing UI strings; debug-only CLI scaffolding is consistent with the established CLI convention. Happy to revisit if you'd prefer the whole CLI surface go through xcstrings.
— Claude Code
There was a problem hiding this comment.
@azooz2003-bit, that's a clear and well-reasoned distinction — CLI diagnostic/error strings and help text following the existing hard-coded English convention, separate from in-app UI localization through xcstrings. I'll record this to avoid raising the same flag on future CLI subcommands.
Would you like me to resolve this comment?
✏️ Learnings added
Learnt from: azooz2003-bit
Repo: manaflow-ai/cmux PR: 4771
File: CLI/cmux.swift:6013-6047
Timestamp: 2026-05-26T05:16:35.015Z
Learning: In the manaflow-ai/cmux project, CLI-facing strings (CLIError messages, per-subcommand help/usage text, and diagnostic summary output in CLI/cmux.swift) are intentionally kept as hard-coded English and are NOT subject to the xcstrings/`String(localized:)` localization requirement. The localization rule (full-internationalization.md) targets in-app user-facing UI strings only. Existing CLI subcommands such as reorder-workspace follow the same hard-coded English pattern. Do not flag missing localization for CLI error messages, help text, or diagnostic output in CLI/**/*.swift.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: CLI/cmux.swift:462-499
Timestamp: 2026-03-09T02:08:51.778Z
Learning: For Claude Code session tag extraction in CLI/cmux.swift, in ClaudeHookTagExtractor.extractTags(subtitle:body:), pre-redact sensitive spans (UUIDs, emails, access tokens, filesystem paths, ENV_VAR=..., long numerics) across the combined body+subtitle using unanchored sensitiveSpanPatterns before tokenization. Then tokenize and still filter each token with anchored sensitivePatterns. Rationale: prevents PII/path fragments from slipping into searchable tags after delimiter splitting.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2514
File: CLI/cmux.swift:9698-9706
Timestamp: 2026-04-01T23:08:15.505Z
Learning: When loading/using a user-provided “custom executable” path in the CLI (e.g., from environment/config/UserDefaults), treat it as an executable file path only if it is NOT a directory and is executable. Concretely, check `isDirectory == false` before checking `isExecutableFile`; this avoids accepting directory-valued paths and should allow safe fallback to PATH or bundled/default executables when the candidate is invalid.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3057
File: CLI/cmux.swift:0-0
Timestamp: 2026-04-24T09:07:05.847Z
Learning: In the CLI error-printing codepaths for setup/install (e.g., in runSetupHooks), when displaying an error, prefer printing `String(describing: error)` instead of `error.localizedDescription`. This ensures the full `CLIError.message` via `CustomStringConvertible` is what shows up in CLI output. Apply this same pattern to similar CLI error prints across the CLI Swift sources.
Learnt from: pgbezerra
Repo: manaflow-ai/cmux PR: 3307
File: Sources/cmuxApp.swift:6413-6417
Timestamp: 2026-04-30T11:55:31.575Z
Learning: In this repo (manaflow-ai/cmux), when adding a new Settings section in SwiftUI (e.g., in Sources/cmuxApp.swift or related Views), don’t wire navigation/search with a raw anchor string alone. Instead: (1) create a corresponding SettingsNavigationTarget enum case (e.g., .workspaces); (2) provide the localized title, symbol, search text, and aliases for that case; (3) add/update the matching entry in SettingsSearchIndex so the sidebar/search can navigate to it; and (4) apply .settingsSearchAnchor(SettingsSearchIndex.sectionID(for: <target>)) to the section header. This prevents broken jump-to behavior by ensuring the navigation anchor and the search index stay consistent.
Learnt from: where-is-atin
Repo: manaflow-ai/cmux PR: 3562
File: CLI/cmux.swift:16408-16419
Timestamp: 2026-05-06T00:14:39.636Z
Learning: Use String(describing: error) instead of error.localizedDescription when formatting errors in the cmux Swift CLI (CLI/**/*.swift). Non-LocalizedError types like CocoaError/NSError can reveal a generic message when using localizedDescription, which hides the real cause. String(describing: error) preserves the full, structured description and is safer for user-facing and diagnostic messages. Apply this in all error formatting paths; if you need a user-friendly message, consider a separate, explicit mapping to user text rather than relying on localizedDescription.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 3626
File: Sources/TabManager.swift:1817-1818
Timestamp: 2026-05-07T08:37:03.967Z
Learning: In this Swift repo (manafow-ai/cmux), when cleaning up stale agent process entries, use `Workspace.clearAgentPID(key:panelId:)` as the single cleanup path. Do not directly mutate `Workspace.statusEntries` or `Workspace.agentPIDs` from outside the dedicated helpers; for example, `TabManager.sweepStaleAgentPIDs` should only call `clearAgentPID` rather than performing its own mutations. This ensures panel-scoped side effects and port/refresh logic run consistently.
Learnt from: psh4607
Repo: manaflow-ai/cmux PR: 3696
File: cmuxTests/ShortcutAndCommandPaletteTests.swift:1716-1771
Timestamp: 2026-05-07T10:56:50.266Z
Learning: In the manaflow-ai/cmux repo, SwiftLint does not enforce a `required_deinit` rule (no project `.swiftlint.yml` in cmux itself, no `required_deinit` in `.github/review-bot-rules/`, and no SwiftLint CI run in `.github/workflows/`). During code reviews, do not raise findings for missing `deinit` on `XCTestCase` subclasses or other Swift classes based on a `required_deinit` rule.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3784
File: README.md:161-161
Timestamp: 2026-05-09T04:48:35.413Z
Learning: In the cmux project, the right-sidebar keyboard shortcut labels were intentionally swapped (per PR `#3784`). Reviewers should NOT flag the ⌘⇧E (Cmd+Shift+E) label as “Open file explorer.” Use these mappings consistently: ⌘⇧E → `focusRightSidebar` with the user-facing label “Toggle right sidebar focus”; ⌘⌥B (Cmd+Option+B) → `toggleFileExplorer` with the user-facing label “Open file explorer.”
The subcommand was wired into the routing and per-command help blocks but missing from the top-level help summary, which is what the paired profile-pr skill greps to detect whether a tagged build carries the workload generator. Without this line the chicken-and-egg guard would flag every build as missing it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Greptile P1 and CodeRabbit minor feedback on #4771. The handler's Thread.sleep was running on the main actor (the method defaulted to .mainActor since it wasn't listed in socketWorkerV2Methods), which would block the UI for the entire simulation and defeat the profiling workload's purpose. Add it to socketWorkerV2Methods, register the case in the worker dispatch switch, mark the handler nonisolated, and route all parameter resolution + state access through a single v2MainSync block so the sleep stays on the worker thread while the SidebarDragState mutations hop to main. Also reject explicit invalid duration_ms/steps with invalid_params instead of silently falling back (v2HasNonNullParam-guarded), and advertise the method in system.capabilities for symmetry with debug.terminal.simulate_file_drop. Validated against the rebuilt sim-drag tag: handler returns the expected JSON payload with the synthesized step path, bad-input gates fire correctly, main thread remains responsive during the sleep loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The handler's drag-state mutations were working correctly (each tick's
`v2MainSync { dragState.dropIndicator = ... }` ran successfully and the
readback confirmed the property was set), but the indicator only ever
appeared at the first tick's position. Diagnosis via diagnostic
cmuxDebugLog probes:
08:07:23.059 sidebar.dragState.sidebar tab=4AC0D (handler set draggedTabId)
08:07:23.060 sidebar.dragState.content tab=4AC0D reason=drag_state_change
08:07:23.212 sidebar.dragClear tab=4AC0D reason=mouse_up_failsafe
08:07:23.219 sidebar.dragState.sidebar tab=nil (failsafe wiped it out)
`SidebarDragFailsafeMonitor.start()` probes `CGEventSource.buttonState`
on the very first `draggedTabId` non-nil transition and posts a
`mouse_up_failsafe` clear if no left button is held — which is true by
construction for a simulator with no NSDraggingSession. After the clear,
`dragState.draggedTabId == nil`, so `SidebarTabDropIndicatorPredicate
.topVisible`'s `guard draggedTabId != nil` returns false for every row
and subsequent dropIndicator mutations produce no visible indicator.
Fix: add `isSimulated: Bool` to SidebarDragState. The
`.onChange(of: dragState.draggedTabId)` lifecycle observer skips
`dragFailsafeMonitor.start()` when the flag is set. The simulate
handler flips the flag on (just before setting draggedTabId) and
off (in the cleanup block).
Verified end-to-end against the rebuilt sim-drag tag with 27
workspaces, 4 ticks over 8s: zero `dragClear` events during the
simulation; row-eval logs (since removed) showed visible=true firing
for each of the 4 distinct indicator positions; four screenshot
captures at +1.5/+4.5/+7.5/+10.5s have distinct SHA hashes
(previously all identical because the failsafe cleared draggedTabId
immediately).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
10003
to
+10008
| modifierKeyMonitor.start() | ||
| dragState.draggedTabId = nil | ||
| dragState.dropIndicator = nil | ||
| #if DEBUG | ||
| SidebarDragStateRegistry.register(windowId: windowId, dragState: dragState) | ||
| #endif |
Contributor
There was a problem hiding this comment.
isSimulated is not reset in onAppear, unlike draggedTabId and dropIndicator. If the sidebar disappears while a simulation is running, onDisappear calls SidebarDragStateRegistry.unregister, which causes the simulation teardown's v2MainSync guard to early-exit — isSimulated is never cleared. When the sidebar reappears using the same SidebarDragState instance, isSimulated is still true, and the next real HID drag will skip SidebarDragFailsafeMonitor entirely, leaving any stuck drag state unrecoverable.
Suggested change
| modifierKeyMonitor.start() | |
| dragState.draggedTabId = nil | |
| dragState.dropIndicator = nil | |
| #if DEBUG | |
| SidebarDragStateRegistry.register(windowId: windowId, dragState: dragState) | |
| #endif | |
| modifierKeyMonitor.start() | |
| dragState.draggedTabId = nil | |
| dragState.dropIndicator = nil | |
| #if DEBUG | |
| dragState.isSimulated = false | |
| SidebarDragStateRegistry.register(windowId: windowId, dragState: dragState) | |
| #endif |
Collaborator
Author
There was a problem hiding this comment.
Fixed in d441637 — applied your suggested onAppear reset of dragState.isSimulated = false for symmetry with the existing onDisappear cleanup. Defensive against the case where onDisappear didn't run (view destroyed mid-simulation, crash) and the @State SidebarDragState carries the flag into a re-mount, bypassing the real-drag failsafe.
— Claude Code
Three findings from `codex exec --skip-git-repo-check` against b847e04..HEAD: 1. The simulate handler's per-tick `v2MainSync { guard ... else { return } }` blocks silently returned if the sidebar unregistered mid-simulation, so the loop kept sleeping and the call still reported `.ok`. Now the start-of-simulation and per-tick blocks return a Bool through v2MainSync; missing registry yields `not_found` (start) or `aborted` (mid-loop) error result instead of a silent no-op. 2. `VerticalTabsSidebar.onDisappear` cleared draggedTabId and dropIndicator but not isSimulated. If a sidebar unmount caught a simulator-driven drag mid-flight, the @State SidebarDragState would carry isSimulated=true into the next mount, bypassing the real-drag failsafe forever. Reset isSimulated=false alongside the other drag fields on disappear. 3. `--steps` was capped to `pathIndices.count`, which silently collapsed the documented 60Hz profiling case (e.g. `--steps 60` over a 4-row span) into 4 ticks. Removed the cap; the resampling formula already handles requestedSteps > path.count by repeating the same target index across multiple ticks — which is exactly the high-frequency SwiftUI invalidation load the profile-pr skill is designed to measure. Verified: `--steps 60` over a 3-row span now returns steps=60, step_interval=33ms, path_len=60. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two findings from codex review of 632b91a: 1. The handler resolved tabIds from self.tabManager, which is the TerminalController's primary TabManager. In multi-window runs, the workspace list could be from a different window than --window --requested, causing spurious "not in window's workspace list" errors or driving indicators against the wrong window's order. Use AppDelegate.shared?.tabManagerFor(windowId:) inside v2MainSync — matches the pattern already used elsewhere in TerminalController (e.g. v2WindowList/v2WorkspaceList). 2. The CLI used the default 15s socket response timeout, but the handler blocks for the simulation's duration (now uncapped via --steps). Long profiling runs would time out client-side while the app keeps mutating dragState. Compute responseTimeout = max(30s, duration_ms/1000 + 10s slack) and pass it through to client.sendV2. Floor of 30s keeps very short simulations comfortably outside the previous default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uncapping --steps left two latent OOM/payload risks for very large profiling runs (e.g. --steps 10_000_000 for hour-long 60Hz traces): 1. stepIndices was pre-materialized as a full [Int] before the loop ran. At 10M steps that's an ~80MB allocation up front, before any simulation has happened. Switched to a stride-aware closure resolveStepIndex(_:) called inline in the loop — same resampling formula, zero pre-allocation. 2. The response payload's "path" was a full UUID-per-step array. For large --steps that produces a giant JSON blob to serialize, send over the unix socket, and parse client-side, with no useful profiling content beyond the first few dozen entries. Cap the sample at pathSampleLimit=64 entries; add path_truncated=true and path_full_size=<steps> when the simulation exceeded the cap. Verified: --steps 10 returns path_len=10 (full); --steps 200 returns path_len=64, path_truncated=true, path_full_size=200. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile P1 finding on 18158f7. Belt-and-suspenders against the recovery path where onDisappear's isSimulated=false reset doesn't run (view destroyed mid-simulation, app crash, etc.) and the @State SidebarDragState carries the flag into a re-mount. Without this, the next real HID drag on a re-mounted sidebar would silently bypass SidebarDragFailsafeMonitor and become unrecoverable if it got stuck. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 31, 2026
This was referenced Jun 9, 2026
This branch was successfully deployed
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
1 participant
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.
DEBUG-only V2 socket method that drives
SidebarDragStatemutations (draggedTabId+dropIndicator) across N steps over a configurable duration, plus a matchingcmux simulate-sidebar-dragCLI. Intended as the deterministic workload generator for a newprofile-prskill in cmuxterm-hq that wraps it withxctraceto compare a PR's HEAD vs its merge base.Mutation-only by design — the architectural claim under test on perf-sensitive sidebar PRs (e.g. #4736) is that
@Observableper-property tracking plusTabItemViewEquatableconformance contains drag invalidation to the dragged row and its indicator overlays. That hot path lives downstream ofdragStatemutations; driving them directly captures the cost we care about while sidestepping flaky NSEvent synthesis.Pieces:
SidebarDragStateRegistry(DEBUG-only) — per-windowIdmap populated inVerticalTabsSidebar.onAppearand cleared in.onDisappear, so the socket handler resolves a livedragStatewithout plumbing throughAppDelegate.v2DebugSidebarSimulateDraginTerminalController— validateswindow_id/from_tab_id/to_tab_id, computes the step path on the main actor, ticksdragStatewithThread.sleepbetween updates (test-only scaffolding, gated#if DEBUGper CLAUDE.md's sleep rule). Never commits a reorder.cmux simulate-sidebar-drag --window --from --to [--duration-ms] [--steps]CLI subcommand + help text.Test plan
cmux simulate-sidebar-drag --window window:1 --from workspace:1 --to workspace:5 --duration-ms 1500from a tag-boundcmux-debug-cli.sh— observe the drag indicator move down the list and clear.profile-prskill end-to-end (paired PR https://github.com/manaflow-ai/cmuxterm-hq/pull/).xctrace record --template SwiftUIagainst a DEBUG build during the simulation and confirm view-body invocations show up in the trace.Paired with: https://github.com/manaflow-ai/cmuxterm-hq pull request for the
profile-prskill (link to be added once that PR is open).🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a DEBUG-only
debug.sidebar.simulate_dragV2 socket method andcmux simulate-sidebar-dragcommand to drive headless sidebar drag-state mutations for profiling. Runs on the socket worker, supports high-frequency steps, handles sidebar unmounts, bypasses the drag failsafe during simulation, and returns a compact result.New Features
debug.sidebar.simulate_drag(V2): Validateswindow_id/from_tab_id/to_tab_id; rejects badduration_ms/steps. Runs on the socket worker (mutations hop to main), drivesdraggedTabId/dropIndicatoracross N steps (resamples when steps > path), then clears. Reportsnot_found/aborted; advertised insystem.capabilities.window_id,from_tab_id,to_tab_id,steps,step_interval_ms,duration_ms,edge,path. Uses a streaming resampler (no large pre-allocation). Capspathto 64 entries and addspath_truncated/path_full_sizefor very large runs.SidebarDragStateRegistry: Per-window live drag-state registry; registered inVerticalTabsSidebar.onAppear, unregistered on disappear (start/tick checks error if missing).SidebarDragState.isSimulatedskipsSidebarDragFailsafeMonitorduring simulation; defensively reset on appear and disappear.Bug Fixes
tabManagerFor(windowId:)) so simulations target the correct window’s workspace list.--duration-ms/--stepsto avoid timing out long runs.Written for commit d441637. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Chores