Fix React Grab Cmd-Shift-G terminal round-trip - #2615
Conversation
|
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 focus‑aware React Grab routing and typed JS bridge messages: browser panels can arm return‑to‑terminal round trips, post structured pasteback notifications, AppDelegate routes pasted content to a preferred terminal panel and waits for readiness; also swaps two shortcut defaults and adds tests for routing and bridge behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant TM as TabManager
participant BP as BrowserPanel
participant AD as AppDelegate
participant TP as TerminalPanel
User->>TM: toggleReactGrabFromCurrentFocus()
TM->>TM: resolveReactGrabShortcutRoute()
TM->>BP: ensure focused browser & requestExplicitWebViewFocus()
BP-->>TM: focus result (true/false)
alt route includes terminal return
TM->>BP: armReactGrabRoundTrip(returnTo: terminalId)
TM->>BP: ensureReactGrabActive() / inject bridge
else no terminal return
TM->>BP: toggleOrInjectReactGrab()
end
BP->>BP: JS posts {type:'copySuccess', content}
BP->>BP: handleReactGrabBridgeMessage(.copySuccess)
BP->>AD: post .reactGrabDidCopySelection (workspace, browserId, returnPanelId, content)
AD->>TM: resolve workspace & panels
AD->>TP: focus target panel (preferredPanelId)
AD->>TP: sendTextWhenReady(content, preferredPanelId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84026f4b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 12 files
You’re at about 96% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
![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
🧹 Nitpick comments (2)
Sources/AppDelegate.swift (1)
6675-6676: Refocus the originating window before pasteback to complete round-trip UX.
focusTab(...)updates panel focus, but it doesn’t guarantee the source terminal window becomes key/frontmost. In cross-window React Grab flows, this can paste “silently” while the user still sees the browser window.🔧 Proposed fix
- manager.focusTab(workspaceId, surfaceId: returnPanelId, suppressFlash: true) - sendTextWhenReady(content, to: workspace) + if let originWindowId = windowId(for: manager) { + _ = focusMainWindow(windowId: originWindowId) + } + manager.focusTab(workspaceId, surfaceId: returnPanelId, suppressFlash: true) + sendTextWhenReady(content, to: workspace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6675 - 6676, After calling manager.focusTab(workspaceId, surfaceId: returnPanelId, suppressFlash: true), ensure the originating terminal/window is made key/frontmost before performing the paste by activating the app/window and waiting for it to be key; then call sendTextWhenReady(content, to: workspace). Concretely: after focusTab, invoke the window-activation API (e.g., NSApp.activate(ignoringOtherApps: true) or the existing manager/window helper that makes the workspace window key) and only once the window is frontmost/key proceed to call sendTextWhenReady using the same workspaceId/returnPanelId/content values to guarantee the paste targets the intended window.Sources/Panels/ReactGrab.swift (1)
139-151: Consider makingtypefield required instead of defaulting to"stateChange".Line 140 defaults missing
typeto"stateChange"for backward compatibility. If all bridge messages now include an explicit type (per the updated JS on lines 259, 262), consider returningnilwhentypeis absent to catch malformed messages early.🔧 Optional: Strict type requirement
init?(body: [String: Any]) { - let type = body["type"] as? String ?? "stateChange" + guard let type = body["type"] as? String else { return nil } switch type {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/ReactGrab.swift` around lines 139 - 151, The initializer init?(body:) currently defaults type to "stateChange" which masks missing/ malformed messages; change it to require the "type" key by guarding let type = body["type"] as? String else { return nil } and then switch on that type string to construct the enum cases .stateChange(isActive:) and .copySuccess(content:), returning nil for unknown types—this ensures absent or malformed "type" values are rejected rather than silently treated as stateChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3277-3308: The method requestExplicitWebViewFocus() currently
calls suppressOmnibarAutofocus(for:) before confirming focus success, allowing a
stale suppression window when the method returns false; change the flow so that
endSuppressWebViewFocusForAddressBar() and clearWebViewFocusSuppression()
remain, but only call suppressOmnibarAutofocus(for:) after focus actually
succeeds (both the immediate window.makeFirstResponder(webView) path and the
async retry path where noteWebViewFocused() is invoked), ensuring suppression is
only applied when noteWebViewFocused()/window.makeFirstResponder(webView) have
succeeded.
In `@Sources/Panels/ReactGrab.swift`:
- Around line 208-229: In handleReactGrabBridgeMessage, the .copySuccess branch
posts raw HTML content (content) to the ReactGrab pasteback notification, which
may include BiDi/zero-width characters; before sending, sanitize the string by
passing it through the same filtered(_:) sanitizer used in
BrowserPickerMessageHandler (or equivalent) and use the filtered result in the
NotificationCenter.post userInfo for ReactGrabPastebackNotificationKey.content;
keep the rest of the logic (clearing round trip, using
pendingReactGrabReturnTargetPanelId and other keys) intact.
In `@Sources/TabManager.swift`:
- Around line 3345-3352: Before calling workspace.focusPanel(browserPanel.id)
save the current focused panel id (workspace.focusedPanelId) into a local
variable, then attempt to focus the browser and call
browserPanel.requestExplicitWebViewFocus(); if that call fails (the guard
branch) call browserPanel.clearReactGrabRoundTrip() and then restore the prior
focus by calling workspace.focusPanel(previousFocusedId) (only if
previousFocusedId is non-nil and different from browserPanel.id) before
returning false so focus isn't left changed on failure.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 6675-6676: After calling manager.focusTab(workspaceId, surfaceId:
returnPanelId, suppressFlash: true), ensure the originating terminal/window is
made key/frontmost before performing the paste by activating the app/window and
waiting for it to be key; then call sendTextWhenReady(content, to: workspace).
Concretely: after focusTab, invoke the window-activation API (e.g.,
NSApp.activate(ignoringOtherApps: true) or the existing manager/window helper
that makes the workspace window key) and only once the window is frontmost/key
proceed to call sendTextWhenReady using the same
workspaceId/returnPanelId/content values to guarantee the paste targets the
intended window.
In `@Sources/Panels/ReactGrab.swift`:
- Around line 139-151: The initializer init?(body:) currently defaults type to
"stateChange" which masks missing/ malformed messages; change it to require the
"type" key by guarding let type = body["type"] as? String else { return nil }
and then switch on that type string to construct the enum cases
.stateChange(isActive:) and .copySuccess(content:), returning nil for unknown
types—this ensures absent or malformed "type" values are rejected rather than
silently treated as stateChange.
🪄 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: 9261935c-d5bc-4fe1-9a78-bc21369e0faa
📒 Files selected for processing (12)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/Panels/BrowserPanel.swiftSources/Panels/ReactGrab.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftweb/data/cmux-shortcuts.ts
Greptile SummaryRemaps React Grab to ⌘⇧G (moving Terminal Find Previous to ⌥⌘G) and adds a terminal→browser round-trip: Cmd+Shift+G from a focused terminal focuses the sole browser pane, activates React Grab, and after
Confidence Score: 4/5Mostly safe to merge; one P1 concern around The feature works correctly in the common synchronous path and has solid unit test coverage. The P1 concern is that Sources/AppDelegate.swift — the Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant TabManager
participant BrowserPanel
participant ReactGrabJS as React Grab (JS)
participant Terminal
User->>AppDelegate: Cmd+Shift+G (terminal focused)
AppDelegate->>TabManager: toggleReactGrabFromCurrentFocus()
TabManager->>TabManager: resolveReactGrabShortcutRoute(panels)
TabManager->>BrowserPanel: armReactGrabRoundTrip(returnTo: terminalId)
TabManager->>BrowserPanel: requestExplicitWebViewFocus()
BrowserPanel-->>TabManager: true
TabManager->>BrowserPanel: ensureReactGrabActive() [async Task]
BrowserPanel->>ReactGrabJS: evaluateJavaScript(inject + activate)
ReactGrabJS-->>BrowserPanel: stateChange(isActive: true)
User->>ReactGrabJS: Selects element
ReactGrabJS-->>BrowserPanel: copySuccess(content)
BrowserPanel->>BrowserPanel: clearReactGrabRoundTrip()
BrowserPanel->>AppDelegate: post .reactGrabDidCopySelection
AppDelegate->>TabManager: focusTab(workspaceId, surfaceId: returnPanelId)
AppDelegate->>Terminal: sendTextWhenReady(content)
Terminal-->>User: Pasted content
Reviews (1): Last reviewed commit: "fix React Grab terminal round-trip" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
You’re at about 96% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:6684">
P2: When a preferred return panel is provided, falling back to `focusedTerminalPanel` can paste React Grab content into the wrong terminal. If the preferred panel is unavailable, this path should fail instead of rerouting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45457947d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
cmux/Sources/Panels/ReactGrab.swift
Lines 271 to 275 in 5eb2016
Clear armed pasteback target when React Grab injection fails
When Cmd+Shift+G is started from a terminal, pendingReactGrabReturnTargetPanelId is armed before injection, but this failure path returns early without clearing it. If script fetch fails (e.g., offline/hash mismatch) or injection previously armed the target and never became active, the stale target can persist and a later copySuccess from a browser-triggered React Grab session will paste into an old terminal unexpectedly. Clear the pending round-trip target on injection failure (and similarly on eval failure) so failed launches cannot leave latent pasteback state behind.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/Panels/ReactGrab.swift (2)
254-262:⚠️ Potential issue | 🟡 MinorFilter invisible scalars out of the pasteback payload.
contentis still forwarded verbatim in the notification payload. Even after authenticating the bridge, strip BiDi override and zero-width characters here so downstream pasteback never sees visually deceptive input.Based on learnings, this repo already filters web-derived text with a shared
dangerousScalarspattern before publishing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/ReactGrab.swift` around lines 254 - 262, The pasteback payload currently forwards `content` verbatim in the NotificationCenter.post call; sanitize it first by stripping BiDi override and zero-width characters using the repository's shared dangerousScalars pattern (the same filter used for web-derived text) and use the sanitized value instead of `content` when populating ReactGrabPastebackNotificationKey.content; update the code around the NotificationCenter.post in ReactGrab.swift (the block referencing workspaceId, id, returnPanelId, and content) to compute a `sanitizedContent` and pass that into the userInfo payload.
161-166:⚠️ Potential issue | 🔴 CriticalGate copySuccess messages on recent native input, and sanitize content before pasteback.
JavaScript in the injected react-grab bridge can forge
copySuccessmessages at any time. WhenpendingReactGrabReturnTargetPanelIdis armed, the handler immediately posts the untrusted content to the pasteback notification chain without any native-input gating or content filtering.Unlike
BrowserPickerMessageHandler(which gates viaNSEvent.addLocalMonitorForEventsand sanitizes web text for BiDi overrides, zero-width characters, and control characters),ReactGrabapplies neither safeguard:
- Trust boundary: Add a one-shot timestamp check similar to
BrowserPickerMessageHandler—record the arm time and reject messages older than 1.0s.- Content sanitization: Filter
contentfor dangerous scalars before posting the notification. Apply the samefiltered(_:)+prefix(200)pattern used in other web-to-native boundaries.Also applies to: 234–262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/ReactGrab.swift` around lines 161 - 166, The ReactGrab bridge currently accepts untrusted copySuccess messages immediately; add a one-shot native-input gating check and sanitize the content before posting pasteback notifications: in userContentController(_:didReceive:) where you parse ReactGrabBridgeMessage, record and check a one-shot timestamp (e.g., pendingReactGrabReturnArmedAt) and reject any copySuccess messages older than 1.0s, and before posting the notification for pendingReactGrabReturnTargetPanelId sanitize the message content using the existing filtered(_:) helper and then limit to prefix(200); apply the same timestamp gating and filtered(_:) + prefix(200) sanitization to the other copySuccess handling block in the file (the block around lines 234–262) so all web-to-native pastebacks use the same defenses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6732-6738: The function resolveTerminalPanelForTextSend currently
falls back to tab.focusedTerminalPanel even when a preferredPanelId was supplied
but no matching panel exists; change its logic so that when a preferredPanelId
is provided you attempt tab.terminalPanel(for: preferredPanelId) and if that
returns nil you return nil (do not fall back), and only use
tab.focusedTerminalPanel when preferredPanelId is nil; update the
resolveTerminalPanelForTextSend implementation accordingly to enforce this
behavior.
---
Duplicate comments:
In `@Sources/Panels/ReactGrab.swift`:
- Around line 254-262: The pasteback payload currently forwards `content`
verbatim in the NotificationCenter.post call; sanitize it first by stripping
BiDi override and zero-width characters using the repository's shared
dangerousScalars pattern (the same filter used for web-derived text) and use the
sanitized value instead of `content` when populating
ReactGrabPastebackNotificationKey.content; update the code around the
NotificationCenter.post in ReactGrab.swift (the block referencing workspaceId,
id, returnPanelId, and content) to compute a `sanitizedContent` and pass that
into the userInfo payload.
- Around line 161-166: The ReactGrab bridge currently accepts untrusted
copySuccess messages immediately; add a one-shot native-input gating check and
sanitize the content before posting pasteback notifications: in
userContentController(_:didReceive:) where you parse ReactGrabBridgeMessage,
record and check a one-shot timestamp (e.g., pendingReactGrabReturnArmedAt) and
reject any copySuccess messages older than 1.0s, and before posting the
notification for pendingReactGrabReturnTargetPanelId sanitize the message
content using the existing filtered(_:) helper and then limit to prefix(200);
apply the same timestamp gating and filtered(_:) + prefix(200) sanitization to
the other copySuccess handling block in the file (the block around lines
234–262) so all web-to-native pastebacks use the same defenses.
🪄 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: ed5d4c23-ebfd-4f60-a080-69b03c8d4552
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/Panels/ReactGrab.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/ReactGrab.swift (1)
279-313:⚠️ Potential issue | 🟠 MajorDon't re-evaluate the react-grab bundle on every re-activation.
toggleOrInjectReactGrab()andensureReactGrabActive()both route back throughinjectReactGrab()wheneverisReactGrabActiveisfalse. IninjectReactGrab(), the JavaScript IIFE checks for an existingwindow.__REACT_GRAB__API and returns early (line 313) if already installed, butscriptSourceis appended outside the IIFE (line 323), meaning the third-party bundle is always re-evaluated regardless of the fast path. Each re-activation re-runs the bundle unnecessarily and risks side effects if the bundle is not fully idempotent. MovescriptSourceevaluation inside the IIFE so it only runs on first install, and invoke onlyactivate()on subsequent toggles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/ReactGrab.swift` around lines 279 - 313, The IIFE in injectReactGrab() currently returns early when window.__REACT_GRAB__ exists but scriptSource is appended outside the IIFE so the third‑party bundle is re-evaluated on every toggle; to fix, move the evaluation of scriptSource inside the IIFE and only append/execute it when installing the bridge (i.e., before or as part of installBridge when window.__REACT_GRAB__ is absent), leaving the early-return path to only call window.__REACT_GRAB__.activate() on subsequent calls from toggleOrInjectReactGrab() / ensureReactGrabActive(); refer to injectReactGrab(), window.__REACT_GRAB__, scriptSource, installBridge and activate() to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 825-826: The code currently calls panel.clearReactGrabRoundTrip()
immediately before Task { await panel.toggleOrInjectReactGrab() }, which can
erase a round-trip target armed via armReactGrabRoundTrip(returnTo:) and cause
copySuccess to have no return panel; change the flow so you do not clear the
round-trip state before toggleOrInjectReactGrab() runs—either remove the
unconditional call to panel.clearReactGrabRoundTrip() here and let
toggleOrInjectReactGrab() clear it when appropriate, or move the clear into the
completion path inside toggleOrInjectReactGrab() (or after confirming no return
panel is needed) so that armReactGrabRoundTrip(returnTo:) remains valid and
copySuccess/pasteback can find the return panel.
In `@Sources/Panels/ReactGrab.swift`:
- Around line 216-250: The pendingReactGrabReturnTargetPanelId is not being
cleared on canceled grabs so stale return targets can be used later; update
handleReactGrabBridgeMessage (and the ReactGrabBridgeMessage enum if needed) to
explicitly clear/disarm pendingReactGrabReturnTargetPanelId on a cancel pathway
(e.g., when receiving a cancel message or when stateChange becomes false), and
also ensure any browser-only activation path clears stale
pendingReactGrabReturnTargetPanelId before arming a new target (use
clearReactGrabRoundTrip() where appropriate); add a regression test that
simulates a canceled terminal-launched grab followed by a browser-initiated copy
to assert no stale paste occurs.
---
Outside diff comments:
In `@Sources/Panels/ReactGrab.swift`:
- Around line 279-313: The IIFE in injectReactGrab() currently returns early
when window.__REACT_GRAB__ exists but scriptSource is appended outside the IIFE
so the third‑party bundle is re-evaluated on every toggle; to fix, move the
evaluation of scriptSource inside the IIFE and only append/execute it when
installing the bridge (i.e., before or as part of installBridge when
window.__REACT_GRAB__ is absent), leaving the early-return path to only call
window.__REACT_GRAB__.activate() on subsequent calls from
toggleOrInjectReactGrab() / ensureReactGrabActive(); refer to injectReactGrab(),
window.__REACT_GRAB__, scriptSource, installBridge and activate() to locate and
update 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8be353b2-7ed3-4ed2-9385-02728b28d77d
📒 Files selected for processing (3)
Sources/Panels/BrowserPanelView.swiftSources/Panels/ReactGrab.swiftcmuxTests/BrowserPanelTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/BrowserPanelTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c28b8a0d91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Panels/ReactGrab.swift (1)
242-270:⚠️ Potential issue | 🟡 MinorSanitize web-derived pasteback content before posting notification.
copySuccesscurrently forwards rawcontentinto pasteback. Please apply the same invisible-scalar filtering pattern used elsewhere for web-originated text before posting.🔧 Proposed fix
extension BrowserPanel { + private func sanitizeReactGrabPastebackContent(_ input: String) -> String { + let dangerousScalars: Set<UInt32> = [ + 0x202A, 0x202B, 0x202C, 0x202D, 0x202E, 0x2066, 0x2067, 0x2068, 0x2069, + 0x200B, 0x200C, 0x200D, 0x200E, 0x200F, 0xFEFF, + ] + let filteredScalars = input.unicodeScalars.filter { scalar in + let v = scalar.value + if dangerousScalars.contains(v) { return false } + return !CharacterSet.controlCharacters.contains(scalar) + } + return String(String.UnicodeScalarView(filteredScalars)) + } + func handleReactGrabBridgeMessage(_ message: ReactGrabBridgeMessage) { switch message { case .stateChange(let isActive): isReactGrabActive = isActive @@ case .copySuccess(let content): guard let returnPanelId = pendingReactGrabReturnTargetPanelId else { @@ clearReactGrabRoundTrip(reason: "copySuccess") + let sanitizedContent = sanitizeReactGrabPastebackContent(content) NotificationCenter.default.post( name: .reactGrabDidCopySelection, object: nil, userInfo: [ ReactGrabPastebackNotificationKey.workspaceId: workspaceId, ReactGrabPastebackNotificationKey.browserPanelId: id, ReactGrabPastebackNotificationKey.returnPanelId: returnPanelId, - ReactGrabPastebackNotificationKey.content: content, + ReactGrabPastebackNotificationKey.content: sanitizedContent, ] ) } } }Based on learnings: “BrowserPickerMessageHandler uses dangerousScalars filtering for web-derived text (
sanitizeWebText/filtered(_)) to strip BiDi/zero-width/control hazards.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/ReactGrab.swift` around lines 242 - 270, The .copySuccess branch forwards raw web-derived content into the pasteback notification; run the content through the existing web-sanitization helper (e.g., sanitizeWebText(...) or the filtered(_:) call that applies dangerousScalars filtering) before using it in debug logs, calling clearReactGrabRoundTrip, or populating NotificationCenter userInfo (ReactGrabPastebackNotificationKey.content). Update the code in the case .copySuccess (around pendingReactGrabReturnTargetPanelId handling) to replace raw content with the sanitized result so all downstream consumers receive the filtered text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 3345-3349: The code focuses the routed browser panel then
immediately calls browserPanel.requestExplicitWebViewFocus(), but if the browser
is in a hidden split-zoomed pane the panel must be revealed first; insert a call
to clear the split-zoom before focusing. Specifically, before calling
workspace.focusPanel(browserPanel.id) (and prior to
browserPanel.requestExplicitWebViewFocus()), invoke the split-zoom clearing
helper (e.g., workspace.clearSplitZoom()) so the browser pane is unhidden, then
call workspace.focusPanel(browserPanel.id) and then
browserPanel.requestExplicitWebViewFocus().
---
Duplicate comments:
In `@Sources/Panels/ReactGrab.swift`:
- Around line 242-270: The .copySuccess branch forwards raw web-derived content
into the pasteback notification; run the content through the existing
web-sanitization helper (e.g., sanitizeWebText(...) or the filtered(_:) call
that applies dangerousScalars filtering) before using it in debug logs, calling
clearReactGrabRoundTrip, or populating NotificationCenter userInfo
(ReactGrabPastebackNotificationKey.content). Update the code in the case
.copySuccess (around pendingReactGrabReturnTargetPanelId handling) to replace
raw content with the sanitized result so all downstream consumers receive the
filtered text.
🪄 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: ff00101e-b0e6-43d2-a30a-91e1b5e1126f
📒 Files selected for processing (5)
Sources/Panels/BrowserPanelView.swiftSources/Panels/ReactGrab.swiftSources/TabManager.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/ShortcutAndCommandPaletteTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 430884bec4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d64aed8ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483de84e26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| Object.defineProperty(window, updaterName, { | ||
| value: syncSessionToken, | ||
| writable: false, | ||
| configurable: false, |
There was a problem hiding this comment.
Fresh evidence for the pasteback-auth issue: this bridge is installed in page JS context and calls Object.defineProperty(window, updaterName, ...) before refreshSessionToken(), so a malicious page can monkey-patch Object.defineProperty first, wrap syncSessionToken, and capture the native session token when it is synced. With that token, page JS can forge cmuxReactGrab copySuccess messages and force arbitrary text into the armed terminal.
Useful? React with 👍 / 👎.
| let browserPanels = panels.filter { $0.panelType == .browser } | ||
| guard browserPanels.count == 1, let browserPanel = browserPanels.first else { | ||
| return nil |
There was a problem hiding this comment.
The terminal path currently requires exactly one browser panel in resolveReactGrabShortcutRoute, but the caller builds snapshots from workspace.panels.values (all tabs), so a workspace with one browser pane containing multiple browser tabs is treated as ambiguous and Cmd+Shift+G from terminal wrongly fails. This contradicts the intended "only browser pane" behavior and blocks valid round-trip routing.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Summary
Cmd+Shift+Gand move terminal find previous off that chordCmd+Shift+Gfrom a terminal route to the only browser pane, start React Grab there, and remember the originating terminal for pastebackTesting
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-cmd-shift-g-react-grab-roundtrip-red test -only-testing:cmuxTests/BrowserDeveloperToolsShortcutDefaultsTests/testDefaultShortcutForToggleReactGrabUsesCommandShiftG(fails on commit08d17ac8)xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-cmd-shift-g-react-grab-roundtrip test -only-testing:cmuxTests/BrowserPanelReactGrabBridgeTests -only-testing:cmuxTests/ReactGrabShortcutRouteTests -only-testing:cmuxTests/BrowserDeveloperToolsShortcutDefaultsTests./scripts/reload.sh --tag task-cmd-shift-g-react-grab-roundtripTask
Summary by cubic
Remaps React Grab to Cmd+Shift+G and moves Terminal “Find Previous” to Option+Cmd+G. From a terminal, Cmd+Shift+G focuses the only browser pane, starts React Grab, then returns to the original terminal and pastes the copied selection.
New Features
web/data/cmux-shortcuts.ts.copySuccesswith a round‑trip token; postsreactGrabDidCopySelectionsoAppDelegatecan refocus the exact terminal and paste.Bug Fixes
Written for commit 483de84. Summary will update on new commits.
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Tests