Repository navigation
Share one terminal workspace with browser guests - #8468
lawrencecchen wants to merge 72 commits into
Conversation
…kspace sharing One ShareSession DO per share code fans host streams (render-grid frames, layout, cursors, chat) out to guest browsers and relays editor-role guest input back to the host. Pure session core + offline Ed25519 token verify, mirroring workers/presence conventions (bun test, hibernation WebSockets, worker-side auth boundary).
Guest path for multiplayer sharing: Stack-gated share page with a cmux-shaped viewer (workspace sidebar with presence dots, split-pane canvas terminals painted from render-grid frames, remote kite cursors, cursor chat bubbles, floating session chat, guest typing for editors), plus /api/share/sessions create + guest-token routes minting Ed25519 JWTs the share worker verifies offline (mirrors the relay token system).
… input forwarding DO relays compose/pointer/webkey (editor-gated, shared-workspace-scoped) to the host; host focus feeds presence for follow-the-host. Web viewer decodes H.264/WebP pixel frames for browser/agent panes, adds the shared composer strip with remote carets, and forwards pointer/keyboard into interactive browser panes.
New Sources/Share/ implements the host side of the cmux share protocol (workers/share/src/protocol.ts): session create + host-token refresh via the web API, a reconnecting WebSocket to the session Durable Object, bonsplit layout serialization, subscriber-driven render-grid streaming, guest input (editor-role gated), chat window with inline access-request moderation, and guest cursor overlays with host cursor broadcast. Palette commands palette.shareWorkspaces / palette.stopSharing wire into ShareSessionController.shared; strings localized en+ja.
…input Browser and agent panes stream to guests as binary pixel frames (kind 0x02): SharePixelStreamer paces up to 10 fps per subscribed pane, skips unchanged frames via a 32x32 downsample digest, and encodes VideoToolbox H.264 Annex B (SPS/PPS inline on keyframes, forced keyframe per new subscriber/resync) with a JPEG-still fallback at ~3 fps; capped at 4 concurrent panes, newest subscription wins. ShareComposerSync makes the host authoritative for agent composer text: guest compose ops are positionally rebased (codepoint indices, rule documented in the file), applied, pushed into the pane via a new composer.setText bridge event, and re-broadcast as compose-state; host typing reaches the sync through a new composer.changed bridge method (both ends added to webviews/src/agent-session and the committed webviews-app bundle regenerated). ShareBrowserInputApplier delivers editor-role guest pointer and key events to BrowserPanel webviews as synthesized NSEvents (wheel via JS on the scrollable under the pointer). guest-sub now routes by pane kind.
Found in live preflight: a rejoining guest's first frame was a delta (blank pane in the viewer) because the host never learned the previous subscriber left, so its per-pane count stayed stale and the new-subscriber full frame was skipped.
The subscription key separator was an accidental literal NUL byte; the disconnect fix split on a space and never matched, so dropped-subscriber counts still failed to reach the host. Name the separator, split on it, and keep the regression tests.
|
Review the following changes in direct dependencies. Learn more about Socket 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:
📝 WalkthroughWalkthroughChangesMultiplayer workspace sharing is added across the native app, web viewer, authenticated session APIs, Cloudflare Durable Object worker, streaming protocol, chat, cursors, terminal input, composer synchronization, localization, documentation, and CI workflows. Multiplayer workspace sharing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant NativeHost
participant WebAPI
participant ShareWorker
participant ShareSession
participant BrowserViewer
NativeHost->>WebAPI: create share session
WebAPI-->>NativeHost: share code and host token
NativeHost->>ShareWorker: open authenticated WebSocket
ShareWorker->>ShareSession: route verified host connection
BrowserViewer->>WebAPI: request guest token
WebAPI-->>BrowserViewer: guest token and WebSocket URL
BrowserViewer->>ShareWorker: open guest WebSocket
ShareWorker->>ShareSession: route verified guest connection
NativeHost->>ShareSession: send layout and binary frames
ShareSession-->>BrowserViewer: session state and streamed frames
BrowserViewer->>ShareSession: send chat, cursor, or editor input
ShareSession-->>NativeHost: relay authorized guest actions
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (7 errors, 2 warnings)
✅ Passed checks (16 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 |
![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: 017dbb406a
ℹ️ 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".
| let core = await this.ensureCore(); | ||
| if (!core) { | ||
| if (!isHost) return new Response("no such session", { status: 404 }); | ||
| const persisted = ShareSessionCore.create(code, { user, email }, Date.now()); |
There was a problem hiding this comment.
When no persisted session exists, this creates it for any connection carrying a host claim. The /api/share/sessions/[code]/token endpoint currently mints host:true for any authenticated caller that posts {"host":true}, and the Mac copies the share URL before socket.start(), so a guest who receives the code and races the host can claim the DO as themselves; the real creator is then rejected by the core.persisted.host.user !== user check. Bind host tokens to the creator or make only the create-time host token able to materialize the DO.
Useful? React with 👍 / 👎.
| for (const c of this.conns.values()) { | ||
| if (!c.isHost && c.active && c.subs.has(key)) { | ||
| effects.push({ kind: "sendBinary", to: c.id, data }); |
There was a problem hiding this comment.
When the host removes a workspace from shared, existing conn.subs entries for panes in that workspace are not cleared, and this routing path only checks the subscription key. Because the browser's shared handler doesn't send unsubs either, any approved guest already viewing that workspace continues receiving live grid/pixel frames after it was unshared until they disconnect or switch, so unshare does not revoke access.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds multiplayer workspace sharing to cmux: a Cloudflare Durable Object (
Confidence Score: 4/5Safe to merge with one defect in the web viewer reconnect logic worth fixing before wide rollout. The auth chain, DO session core, and macOS host input validation are solid. The one current defect is in share-connection.ts: any non-OK HTTP response from the Vercel token endpoint (including transient 5xx) permanently sets the viewer to unavailable with no self-healing, while only thrown network exceptions trigger the reconnect backoff path. Additionally, several macOS-side issues from previous reviews (ObservableObject broad invalidation, singleton ambient-state in closures, Task.sleep backoff) remain open. web/app/[locale]/share/[code]/share-connection.ts (token fetch error handling), Sources/Share/ShareSessionController.swift and Sources/Share/ShareSocket.swift (see prior review threads) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Mac as macOS Host
participant WebAPI as Web API (Vercel)
participant Worker as Share Worker (CF)
participant DO as ShareSession DO
participant Guest as Guest Browser
Mac->>WebAPI: POST /api/share/sessions (native auth)
WebAPI-->>Mac: "{ code, token(host+create), wsUrl, shareUrl }"
Mac->>Worker: "WS connect ?token=JWT"
Worker->>Worker: verifyShareToken offline
Worker->>DO: "fetch (x-share-host=1, x-share-create=1)"
DO->>DO: ShareSessionCore.create() + persist
DO-->>Mac: session-state snapshot
Guest->>WebAPI: "POST /api/share/sessions/{code}/token (cookie auth)"
WebAPI-->>Guest: "{ token(guest), wsUrl }"
Guest->>Worker: "WS connect ?token=guest JWT"
Worker->>DO: "fetch (x-share-host=0)"
DO-->>Guest: access-pending
DO-->>Mac: access-request
Mac->>DO: "approve { user, role }"
DO-->>Guest: session-state snapshot
Mac->>DO: binary grid/pixel frame
DO->>DO: routeBinary to subscribed guests
DO-->>Guest: binary frame fan-out
Guest->>DO: "input { ws, pane, data }"
DO->>DO: validate editor role + shared workspace
DO-->>Mac: guest-input
Mac->>Mac: applyGuestInput (re-validate locally)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Mac as macOS Host
participant WebAPI as Web API (Vercel)
participant Worker as Share Worker (CF)
participant DO as ShareSession DO
participant Guest as Guest Browser
Mac->>WebAPI: POST /api/share/sessions (native auth)
WebAPI-->>Mac: "{ code, token(host+create), wsUrl, shareUrl }"
Mac->>Worker: "WS connect ?token=JWT"
Worker->>Worker: verifyShareToken offline
Worker->>DO: "fetch (x-share-host=1, x-share-create=1)"
DO->>DO: ShareSessionCore.create() + persist
DO-->>Mac: session-state snapshot
Guest->>WebAPI: "POST /api/share/sessions/{code}/token (cookie auth)"
WebAPI-->>Guest: "{ token(guest), wsUrl }"
Guest->>Worker: "WS connect ?token=guest JWT"
Worker->>DO: "fetch (x-share-host=0)"
DO-->>Guest: access-pending
DO-->>Mac: access-request
Mac->>DO: "approve { user, role }"
DO-->>Guest: session-state snapshot
Mac->>DO: binary grid/pixel frame
DO->>DO: routeBinary to subscribed guests
DO-->>Guest: binary frame fan-out
Guest->>DO: "input { ws, pane, data }"
DO->>DO: validate editor role + shared workspace
DO-->>Mac: guest-input
Mac->>Mac: applyGuestInput (re-validate locally)
Reviews (4): Last reviewed commit: "Extract spliceDiff into a pure module" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| cursorOverlay.isWorkspaceVisible = { [weak self] ws in | ||
| guard let self, let tabManager = self.tabManager, | ||
| let wsUUID = UUID(uuidString: ws) else { return false } | ||
| return tabManager.selectedTabId == wsUUID | ||
| } | ||
| cursorOverlay.sendHostCursor = { [weak self] pos in |
There was a problem hiding this comment.
String(describing: error) on a thrown network/HTTP error will produce strings like "NSURLErrorDomain Code=-1009 …" or stack-style descriptions that name internal types and domains. Since lastErrorText is @Published and shown in the UI, this violates the user-facing error message policy. The same pattern at line 812 (lastErrorText = "\(code): \(message)") directly exposes internal worker error key strings (e.g. share_not_configured: …) to users.
Both sites should map the error to a user-friendly cmux-terms message, keeping the raw code/detail in a debug log only.
|
|
||
| private func rebuildFeed(chat: [ShareChatMessage]) { | ||
| var items = chat.map(feedItem(for:)) |
There was a problem hiding this comment.
ObservableObject causes broad SwiftUI invalidation
ShareSessionController is marked @MainActor final class and conforms to ObservableObject with several @Published properties (participants, feed, workspaceRows, status, …). With ObservableObject, any single @Published change — including high-frequency cursor-driven participants updates — invalidates the entire ShareChatView body and all its child views. Swift's @Observable macro would give property-granular invalidation so only views that actually read the changed property rerender.
| private func rebuildFeed(chat: [ShareChatMessage]) { | ||
| var items = chat.map(feedItem(for:)) |
There was a problem hiding this comment.
static let shared singleton for runtime state
ShareSessionController.shared is a new static let shared singleton for orchestrating the entire sharing session lifecycle. ShareChatView correctly receives the controller as @ObservedObject var controller, but the value-type sub-views (ShareWorkspaceToggleRow, ShareFeedRow, ShareParticipantRow) ignore the injected reference and reach directly into ShareSessionController.shared from their action closures, creating a second ambient access path alongside the injected one and making the views non-testable in isolation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@docs/multiplayer-share.md`:
- Around line 33-36: Add the text language identifier to the fenced ASCII
diagram code block in the multiplayer sharing documentation, changing the
opening fence to specify text while preserving the diagram content.
In `@Sources/Share/ContentView`+ShareCommandPalette.swift:
- Around line 16-35: Add catalog translations for command.shareWorkspaces.title,
command.stopSharing.title, and command.share.subtitle in
Resources/Localizable.xcstrings for every supported locale beyond en and ja.
Preserve the existing localization keys and provide appropriate localized values
for each shipped language.
In `@Sources/Share/ShareSessionController.swift`:
- Around line 100-110: Update the error handling in
Sources/Share/ShareSessionController.swift lines 100-110 to log the raw error
separately and assign a localized product-level message via
String(localized:defaultValue:). Also update lines 413-414 to map known error
codes to localized user-facing copy, while logging the raw code and message
instead of exposing them through lastErrorText.
- Around line 33-49: Update ShareSessionController to use the Observation
framework’s `@Observable` model instead of ObservableObject and `@Published` for its
state properties. Preserve the existing status, shareUrl, code, participants,
feed, workspaceRows, and lastErrorText access behavior, and keep Combine
throttle usage only at publisher boundaries.
- Around line 82-94: Add entries for share.error.noWorkspaceContext and
share.error.notSignedIn to every supported locale in
Resources/Localizable.xcstrings, preserving the existing English and Japanese
translations and matching the catalog’s established localization structure.
In `@web/app/`[locale]/share/[code]/pixel-pane.ts:
- Around line 19-20: Replace the unbounded stillChain queue with
single-in-flight decode behavior so a new frame supersedes any pending stale
frame instead of extending the promise chain. Update the still-frame decode and
setImage flow around push/close to track closed state, prevent post-close
setImage calls, and ensure pending work is discarded when close() runs.
In `@web/app/`[locale]/share/[code]/share-panes.tsx:
- Around line 239-247: Update the interactive browser pane’s onKeyDown handler
to detect Escape and blur the focused tabIndex={0} overlay, returning before
preventDefault and web-key forwarding. Preserve the existing meta-key shortcut
behavior and forwarding for all other keys.
In `@web/app/api/share/sessions/route.ts`:
- Around line 46-70: Update handleShareSessionCreate to enforce a rate limit
before generating the session code or minting the token, using the authenticated
user or request IP as the limiter key and the existing dependency/configuration
pattern for shared state. Return the established rate-limit response when the
limit is exceeded while preserving the current unauthorized and
share_not_configured responses.
In `@workers/share/src/index.ts`:
- Around line 41-63: Remove the incoming Authorization header from the forwarded
headers in the request-building flow after token verification, while preserving
the verified x-share-* identity headers and existing URL behavior. Update the
headers handling around the forward request before calling stub.fetch so the
bearer token never reaches the Durable Object.
In `@workers/share/src/session.ts`:
- Around line 418-422: Bound subscription growth in the "sub" branch of the
session message handler: validate msg.pane against the workspace’s known layout
panes or enforce a fixed maximum on conn.subs before adding subKey(msg.ws,
msg.pane). Ignore or reject new subscriptions beyond the bound while preserving
existing subscriptions and subCountChanged behavior.
🪄 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: 21317dc2-2a72-4040-8192-8f8c8e30757c
⛔ Files ignored due to path filters (1)
workers/share/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
.github/workflows/share.ymlResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/agentSessionSurface.mjsSources/ContentView.swiftSources/Panels/AgentSessionWebRendererCoordinator.swiftSources/Panels/AgentSessionWebRendererSession.swiftSources/Share/ContentView+ShareCommandPalette.swiftSources/Share/ShareBrowserInputApplier.swiftSources/Share/ShareChatView.swiftSources/Share/ShareChatWindowController.swiftSources/Share/ShareComposerSync.swiftSources/Share/ShareCursorOverlayController.swiftSources/Share/ShareCursorPointerView.swiftSources/Share/ShareGridStreamer.swiftSources/Share/ShareLayoutSerializer.swiftSources/Share/SharePixelEncoder.swiftSources/Share/SharePixelStreamer.swiftSources/Share/ShareProtocolModels.swiftSources/Share/ShareSessionAPI.swiftSources/Share/ShareSessionController+PaneStreams.swiftSources/Share/ShareSessionController.swiftSources/Share/ShareSocket.swiftcmux.xcodeproj/project.pbxprojdocs/multiplayer-share.mdweb/.env.exampleweb/app/[locale]/share/[code]/layout.tsxweb/app/[locale]/share/[code]/loading.tsxweb/app/[locale]/share/[code]/page.tsxweb/app/[locale]/share/[code]/pixel-pane.tsweb/app/[locale]/share/[code]/share-chat.tsxweb/app/[locale]/share/[code]/share-colors.tsweb/app/[locale]/share/[code]/share-connection.tsweb/app/[locale]/share/[code]/share-cursors.tsxweb/app/[locale]/share/[code]/share-panes.tsxweb/app/[locale]/share/[code]/share-protocol.tsweb/app/[locale]/share/[code]/share-viewer.tsxweb/app/[locale]/share/[code]/shared-composer.tsxweb/app/[locale]/share/[code]/terminal-grid.tsweb/app/[locale]/share/[code]/terminal-keys.tsweb/app/[locale]/share/[code]/use-store.tsweb/app/api/share/sessions/[code]/token/route.tsweb/app/api/share/sessions/route.tsweb/messages/en.jsonweb/messages/ja.jsonweb/services/share/token.tsweb/tests/share-token-route.test.tsweb/tests/share-viewer-logic.test.tswebviews/src/agent-session/react/main.tswebviews/src/agent-session/shared/sessionModel.tswebviews/src/agent-session/shared/types.tsworkers/share/.gitignoreworkers/share/PROTOCOL.mdworkers/share/README.mdworkers/share/package.jsonworkers/share/scripts/dev-proof.tsworkers/share/scripts/guest-probe.tsworkers/share/src/do.tsworkers/share/src/index.tsworkers/share/src/jwt.tsworkers/share/src/protocol.tsworkers/share/src/session.tsworkers/share/test/browser-input.test.tsworkers/share/test/compose.test.tsworkers/share/test/jwt.test.tsworkers/share/test/protocol.test.tsworkers/share/test/session.test.tsworkers/share/tsconfig.jsonworkers/share/tsconfig.test.jsonworkers/share/wrangler.dev.tomlworkers/share/wrangler.toml
| CommandPaletteCommandContribution( | ||
| commandId: "palette.shareWorkspaces", | ||
| title: constant(String( | ||
| localized: "command.shareWorkspaces.title", | ||
| defaultValue: "Share Workspaces…" | ||
| )), | ||
| subtitle: constant(String(localized: "command.share.subtitle", defaultValue: "Share")), | ||
| keywords: ["share", "multiplayer", "collab", "collaborate", "invite", "session", "live"] | ||
| ), | ||
| CommandPaletteCommandContribution( | ||
| commandId: "palette.stopSharing", | ||
| title: constant(String( | ||
| localized: "command.stopSharing.title", | ||
| defaultValue: "Stop Sharing" | ||
| )), | ||
| subtitle: constant(String(localized: "command.share.subtitle", defaultValue: "Share")), | ||
| keywords: ["share", "multiplayer", "stop", "end", "session"], | ||
| when: { $0.bool(CommandPaletteContextKeys.shareSessionActive) }, | ||
| enablement: { $0.bool(CommandPaletteContextKeys.shareSessionActive) } | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify each new share command key exists in the string catalog.
fd -t f 'Localizable.xcstrings' Resources | while read -r f; do
echo "== $f =="
for k in command.shareWorkspaces.title command.stopSharing.title command.share.subtitle; do
if jq -e --arg k "$k" '.strings[$k]' "$f" >/dev/null 2>&1; then
echo "present: $k"
jq --arg k "$k" '.strings[$k].localizations | keys' "$f"
else
echo "MISSING: $k"
fi
done
doneRepository: manaflow-ai/cmux
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat <<'PY' > /tmp/inspect_xcstrings.py
import json, pathlib
p = pathlib.Path("Resources/Localizable.xcstrings")
data = json.loads(p.read_text())
print("catalog localizations:", sorted(data.get("sourceLanguage", "") and [data.get("sourceLanguage")] or []))
# xcstrings schema varies; print top-level keys and any locale-like keys under variations
print("top-level keys:", sorted(data.keys()))
strings = data.get("strings", {})
locales = set()
for k, v in strings.items():
locs = v.get("localizations", {})
locales.update(locs.keys())
print("all locales found in strings:", sorted(locales))
for k in ["command.shareWorkspaces.title", "command.stopSharing.title", "command.share.subtitle"]:
print(f"{k}:")
v = strings.get(k)
if not v:
print(" MISSING")
continue
for loc, locv in sorted(v.get("localizations", {}).items()):
print(f" {loc}: state={locv.get('state')} variations={list(locv.keys())}")
PY
python3 /tmp/inspect_xcstrings.pyRepository: manaflow-ai/cmux
Length of output: 745
Add translations for the new share command-palette strings
command.shareWorkspaces.title, command.stopSharing.title, and command.share.subtitle only have en and ja entries in Resources/Localizable.xcstrings. Add the remaining locale translations in the catalog so these user-facing strings are covered everywhere the app ships.
🤖 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 `@Sources/Share/ContentView`+ShareCommandPalette.swift around lines 16 - 35,
Add catalog translations for command.shareWorkspaces.title,
command.stopSharing.title, and command.share.subtitle in
Resources/Localizable.xcstrings for every supported locale beyond en and ja.
Preserve the existing localization keys and provide appropriate localized values
for each shipped language.
Source: Path instructions
| final class ShareSessionController: ObservableObject { | ||
| static let shared = ShareSessionController() | ||
|
|
||
| enum Status: Equatable { | ||
| case idle | ||
| case starting | ||
| case active | ||
| case reconnecting | ||
| } | ||
|
|
||
| @Published private(set) var status: Status = .idle | ||
| @Published private(set) var shareUrl: String? | ||
| @Published private(set) var code: String? | ||
| @Published private(set) var participants: [ShareParticipant] = [] | ||
| @Published private(set) var feed: [ShareFeedItem] = [] | ||
| @Published private(set) var workspaceRows: [ShareWorkspaceRow] = [] | ||
| @Published private(set) var lastErrorText: String? |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Prefer @Observable over ObservableObject/@Published for this new state.
New cmux-owned UI state should use @Observable rather than introducing an ObservableObject with @Published properties; the existing Combine throttle pipelines can remain at the publisher boundaries.
As per coding guidelines ("Avoid introducing Combine constructs such as ObservableObject, @Published ... for app state ... prefer @Observable").
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 33-33: Classes should have an explicit deinit method
(required_deinit)
🤖 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 `@Sources/Share/ShareSessionController.swift` around lines 33 - 49, Update
ShareSessionController to use the Observation framework’s `@Observable` model
instead of ObservableObject and `@Published` for its state properties. Preserve
the existing status, shareUrl, code, participants, feed, workspaceRows, and
lastErrorText access behavior, and keep Combine throttle usage only at publisher
boundaries.
| lastErrorText = String( | ||
| localized: "share.error.noWorkspaceContext", | ||
| defaultValue: "Sharing is unavailable: no workspace window is open." | ||
| ) | ||
| return | ||
| } | ||
| guard let coordinator = AppDelegate.shared?.auth?.coordinator else { | ||
| lastErrorText = String( | ||
| localized: "share.error.notSignedIn", | ||
| defaultValue: "Sign in to cmux to share workspaces." | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify new share.* localization keys exist in the catalog and enumerate locales.
fd -a 'Localizable.xcstrings' Resources | while read -r f; do
echo "== $f =="
echo "-- share.error keys present --"
jq -r '.strings | keys[] | select(startswith("share."))' "$f" 2>/dev/null | rg 'share\.error' || echo "MISSING share.error.* keys"
echo "-- locales referenced anywhere in catalog --"
jq -r '[.strings[].localizations // {} | keys[]] | unique[]' "$f" 2>/dev/null
echo "-- locales present for share.error.noWorkspaceContext --"
jq -r '.strings["share.error.noWorkspaceContext"].localizations // {} | keys[]' "$f" 2>/dev/null || echo "key absent"
doneRepository: manaflow-ai/cmux
Length of output: 478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
catalog="Resources/Localizable.xcstrings"
python3 - <<'PY'
import json
from pathlib import Path
catalog = Path("Resources/Localizable.xcstrings")
data = json.loads(catalog.read_text())
keys = [k for k in data.get("strings", {}).keys() if k.startswith("share.error.")]
print(f"catalog: {catalog}")
print("supported locales:")
all_locales = sorted({loc for entry in data["strings"].values() for loc in (entry.get("localizations") or {}).keys()})
print(", ".join(all_locales))
for key in keys:
locales = sorted((data["strings"][key].get("localizations") or {}).keys())
print(f"{key}: {', '.join(locales) if locales else '(none)'}")
PYRepository: manaflow-ai/cmux
Length of output: 377
Add the new share error strings to every locale
share.error.noWorkspaceContext and share.error.notSignedIn only have en and ja entries in Resources/Localizable.xcstrings; add the remaining supported locales so these errors don't ship English-only in other languages.
🤖 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 `@Sources/Share/ShareSessionController.swift` around lines 82 - 94, Add entries
for share.error.noWorkspaceContext and share.error.notSignedIn to every
supported locale in Resources/Localizable.xcstrings, preserving the existing
English and Japanese translations and matching the catalog’s established
localization structure.
Source: Path instructions
| startTask = Task { @MainActor [weak self] in | ||
| do { | ||
| let created = try await api.createSession() | ||
| guard let self, self.status == .starting else { return } | ||
| self.activate(created: created, api: api) | ||
| } catch { | ||
| guard let self, self.status == .starting else { return } | ||
| self.status = .idle | ||
| self.lastErrorText = String(describing: error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
User-facing lastErrorText is populated with unlocalized, potentially internal error text. lastErrorText is rendered in the share UI (set with localized share.error.* strings elsewhere), but two paths bypass that: they emit raw/verbatim error strings, which are unlocalized and can leak implementation details. Log the raw values and assign a localized, product-level message.
Sources/Share/ShareSessionController.swift#L100-L110: replacelastErrorText = String(describing: error)with a localizedString(localized:defaultValue:)message and log the underlyingerrorseparately.Sources/Share/ShareSessionController.swift#L413-L414: replacelastErrorText = "\(code): \(message)"with localized copy mapped from knowncodes, keeping the rawcode/messagein logs only.
As per coding guidelines (localize user-facing text; do not expose raw upstream error messages).
📍 Affects 1 file
Sources/Share/ShareSessionController.swift#L100-L110(this comment)Sources/Share/ShareSessionController.swift#L413-L414
🤖 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 `@Sources/Share/ShareSessionController.swift` around lines 100 - 110, Update
the error handling in Sources/Share/ShareSessionController.swift lines 100-110
to log the raw error separately and assign a localized product-level message via
String(localized:defaultValue:). Also update lines 413-414 to map known error
codes to localized user-facing copy, while logging the raw code and message
instead of exposing them through lastErrorText.
| export async function handleShareSessionCreate( | ||
| request: Request, | ||
| deps: ShareSessionCreateDeps, | ||
| ): Promise<Response> { | ||
| const user = await deps.verifyRequest(request); | ||
| if (!user) return unauthorized(); | ||
| const key = deps.signingKey(); | ||
| if (!key) return json({ error: "share_not_configured" }, 503); | ||
| const code = deps.generateCode(); | ||
| const { token, expiresAt } = mintShareToken({ | ||
| sub: user.id, | ||
| email: user.primaryEmail ?? "", | ||
| code, | ||
| host: true, | ||
| key, | ||
| nowSeconds: deps.nowSeconds(), | ||
| }); | ||
| return json({ | ||
| code, | ||
| token, | ||
| expiresAt, | ||
| wsUrl: shareSessionWsUrl(code), | ||
| shareUrl: sharePageUrl(code), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider rate-limiting session creation.
Each authenticated POST mints a fresh code and, once the host connects, a new ShareSession Durable Object. Nothing here bounds how many sessions one user can spin up. Worth a rate limit (per-user or per-IP) at this route or at the edge, since DO instances/tokens are otherwise "free" to mint repeatedly.
🤖 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 `@web/app/api/share/sessions/route.ts` around lines 46 - 70, Update
handleShareSessionCreate to enforce a rate limit before generating the session
code or minting the token, using the authenticated user or request IP as the
limiter key and the existing dependency/configuration pattern for shared state.
Return the established rate-limit response when the limit is exceeded while
preserving the current unauthorized and share_not_configured responses.
…caps, error hygiene - Only create-endpoint tokens may materialize a session in the DO; a host-claim refresh token reconnects but can never squat an unminted code (verified live: 404). - Unsharing a workspace prunes guest subscriptions, notifies the host, and routeBinary drops frames for unshared workspaces. - Per-connection subscription cap (64) bounds guest-supplied state. - The worker strips Authorization/Cookie before forwarding to the DO. - Web still-frame decoding is latest-wins and stops after close(); Escape releases keyboard focus from interactive browser panes. - Mac error surfaces are localized (en+ja); raw error detail goes to the debug log only.
|
Addressed the substantive review findings in 6aa0435: session creation is now gated on a create-endpoint token claim (a host refresh token can reconnect but never squat an unminted code — verified live against the dev worker, 404), unsharing a workspace prunes guest subscriptions and stops frame routing, per-connection subscriptions are capped at 64, the worker strips Authorization/Cookie before forwarding upgrades to the DO, web still-frame decoding is latest-wins and halts on close, Escape releases keyboard focus from interactive browser panes, and Mac-side error surfaces are localized with detail going to the debug log. Declined for now: @observable migration and the singleton note (matches the codebase's current patterns; happy to revisit), and session-create rate limiting (follow-up alongside the relay-token precedent). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Share/ShareSessionController.swift (1)
453-456: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid full-collection scans for chat deduplication.
The chat feed is a scalable collection that can grow continuously during an active session. Calling
feed.contains(where:)on every incoming message results in O(N²) quadratic scaling and violates algorithmic complexity guidelines for hot paths.Please use a
Setof known message IDs for O(1) deduplication. As per path instructions, prefer Set/dictionary/indexed lookups over repeated array scans for scalable collections like chat feeds.🛠️ Proposed fix
Add a private
Setto track known IDs, updating it here and ensuring you clear/repopulate it inrebuildFeedandteardownSession:- private func appendChat(_ message: ShareChatMessage) { - guard !feed.contains(where: { $0.id == message.id }) else { return } - feed.append(feedItem(for: message)) - } + private var knownChatIDs = Set<String>() + + private func appendChat(_ message: ShareChatMessage) { + guard !knownChatIDs.contains(message.id) else { return } + knownChatIDs.insert(message.id) + feed.append(feedItem(for: message)) + }🤖 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 `@Sources/Share/ShareSessionController.swift` around lines 453 - 456, Replace the full-feed scan in appendChat(_:) with a private Set of known chat message IDs, inserting each newly accepted ID before appending its feed item. Keep the set synchronized by clearing and repopulating it when rebuildFeed executes, and clearing it during teardownSession.Source: Path instructions
🤖 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.
Outside diff comments:
In `@Sources/Share/ShareSessionController.swift`:
- Around line 453-456: Replace the full-feed scan in appendChat(_:) with a
private Set of known chat message IDs, inserting each newly accepted ID before
appending its feed item. Keep the set synchronized by clearing and repopulating
it when rebuildFeed executes, and clearing it during teardownSession.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a1378c8-e02d-4d68-bd1e-04c831bdd689
📒 Files selected for processing (14)
Resources/Localizable.xcstringsSources/Share/ShareSessionController.swiftdocs/multiplayer-share.mdweb/app/[locale]/share/[code]/pixel-pane.tsweb/app/[locale]/share/[code]/share-panes.tsxweb/app/api/share/sessions/route.tsweb/services/share/token.tsworkers/share/scripts/dev-proof.tsworkers/share/src/do.tsworkers/share/src/index.tsworkers/share/src/jwt.tsworkers/share/src/session.tsworkers/share/test/jwt.test.tsworkers/share/test/session.test.ts
CI's agent-session-web-resources job rebuilds Resources/agent-session-* from webviews/src/agent-session/shared, which slice 2 changed; the committed bundles were stale (the earlier local build skipped the root frozen-lockfile install the script needs).
| private chat(conn: Conn, text: string, bubble: CursorPos | undefined): Effect[] { | ||
| const trimmed = text.slice(0, CHAT_TEXT_LIMIT).trim(); | ||
| if (!trimmed) return []; | ||
| const msg: ChatMessage = { | ||
| id: crypto.randomUUID(), | ||
| user: conn.user, | ||
| text: trimmed, | ||
| ts: Date.now(), | ||
| ...(bubble ? { bubble } : {}), | ||
| }; | ||
| this.s.chat.push(msg); | ||
| if (this.s.chat.length > CHAT_HISTORY_LIMIT) { | ||
| this.s.chat = this.s.chat.slice(-CHAT_HISTORY_LIMIT); | ||
| } | ||
| return [{ kind: "persist" }, ...this.broadcastActive({ t: "chat", msg }, null)]; | ||
| } |
There was a problem hiding this comment.
cursorBroadcast scrubs cursor positions to null when pos.ws is not a shared workspace, but the chat handler broadcasts bubble verbatim. If a workspace is unshared mid-session and a guest sends a chat with a stale bubble.ws, the DO fans it out to all participants with an invalid workspace reference. On the web client, showBubble stores msg.bubble as the cursor position fallback, so the bubble text silently fails to render for all recipients (the cursor layer filters by activeWs). A malicious editor can also deliberately suppress another participant's bubble display by sending any non-matching workspace ID.
The same scrubbing used in cursorBroadcast should be applied to the bubble field before it is stored in the chat history.
| private chat(conn: Conn, text: string, bubble: CursorPos | undefined): Effect[] { | |
| const trimmed = text.slice(0, CHAT_TEXT_LIMIT).trim(); | |
| if (!trimmed) return []; | |
| const msg: ChatMessage = { | |
| id: crypto.randomUUID(), | |
| user: conn.user, | |
| text: trimmed, | |
| ts: Date.now(), | |
| ...(bubble ? { bubble } : {}), | |
| }; | |
| this.s.chat.push(msg); | |
| if (this.s.chat.length > CHAT_HISTORY_LIMIT) { | |
| this.s.chat = this.s.chat.slice(-CHAT_HISTORY_LIMIT); | |
| } | |
| return [{ kind: "persist" }, ...this.broadcastActive({ t: "chat", msg }, null)]; | |
| } | |
| const scrubbedBubble = bubble && this.isSharedWorkspace(bubble.ws) ? bubble : undefined; | |
| const msg: ChatMessage = { | |
| id: crypto.randomUUID(), | |
| user: conn.user, | |
| text: trimmed, | |
| ts: Date.now(), | |
| ...(scrubbedBubble ? { bubble: scrubbedBubble } : {}), | |
| }; |
Keeps the viewer-logic test framework-free: importing the composer component pulled next-intl (and next/navigation) into bun's test module graph, the only plausible source of CI's RedirectType interop error.
| }); | ||
| if (!res.ok) { | ||
| this.session.update({ status: "unavailable" }); | ||
| return; | ||
| } | ||
| grant = (await res.json()) as { token: string; wsUrl: string }; | ||
| } catch { | ||
| this.scheduleReconnect(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Any non-OK HTTP status from POST /api/share/sessions/${code}/token — including transient 5xx errors from Vercel — unconditionally sets status: "unavailable" and returns without scheduling a reconnect. Only thrown network exceptions reach scheduleReconnect. If the token endpoint returns a transient 500 or 502, the viewer is permanently stuck at "Session unavailable" with no self-healing path; the user must manually refresh.
The current distinction collapses a continuum of error types — definite terminal states (e.g. 400 invalid code), auth errors (401), and configuration errors (503 for missing key) — alongside genuinely transient infrastructure failures (500/502). Only the first three categories warrant a permanent stop; the last should schedule a retry just as a network exception does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8733adea1
ℹ️ 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".
| guard let taskToStop = webSocketTask, !isStopped else { | ||
| if connection == nil { | ||
| await stop() | ||
| return true | ||
| } |
There was a problem hiding this comment.
If the user chooses Stop Sharing while the host socket is reconnecting, webSocketTask is nil, so this branch reports success after only stopping the local socket and never sends the .end message. The Durable Object therefore treats this as an ordinary disconnect and keeps the guest sockets, grants, chat, and invitation valid for the two-minute host grace period, even though the Mac has returned to the idle state and indicates sharing stopped. This path needs an authenticated server-side revocation or must reconnect long enough to deliver the end command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd663240c
ℹ️ 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".
| guard let workspace = sharedWorkspace(in: tabManager) else { | ||
| teardownSession() |
There was a problem hiding this comment.
When the host closes the shared workspace while its window remains open, this path calls teardownSession() without .end. The socket is therefore closed as an ordinary disconnect, so the relay retains the previous workspace state, guest connections, chat, and invitation for the two-minute host grace period even though the Mac reports sharing as idle. Pass the final end message through this automatic teardown path so removing the shared workspace revokes the session immediately.
Useful? React with 👍 / 👎.
| self.status = .active | ||
| _ = self.sendHello() |
There was a problem hiding this comment.
When terminal output occurs while the host socket is disconnected, ShareGridStreamer rejects the send and marks that pane as needing a baseline. A normal socket reconnect reaches this .opened branch, but it only replays hello; the worker's connectHost path sends a snapshot without a resync, and the controller only calls streamer.resendFullFrames() when handling an explicit relay resync. If the terminal becomes idle before reconnection, guests remain on the pre-disconnect screen until some later Ghostty tick or output happens to trigger capture, so opening a replacement connection should explicitly schedule fresh baselines for existing subscriber streams.
Useful? React with 👍 / 👎.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 419349620c
ℹ️ 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".
| CloudVMActionLauncher.shared.terminateAll() | ||
| CmuxSSHURLProcessLauncher.shared.terminateAll() | ||
| MobileHostService.shared.stop() | ||
| shareSessionController.stopSharing() |
There was a problem hiding this comment.
When quitting while a share is active, this synchronous delegate call only causes teardownSession to schedule a new main-actor socketStopTask; it does not await ShareSocket.sendAndStop(.end). The applicationShouldTerminate owned-cleanup path does not include sharing, so the process can exit immediately after this callback before the task runs, leaving guests and the invitation active until the two-minute host-disconnect grace period expires. Include share revocation in the bounded terminateLater cleanup and await it before replying.
Useful? React with 👍 / 👎.
| guard CmuxFeatureFlags.shared.isMultiplayerShareUIEnabled else { return } | ||
| shareSessionController.stopSharing() |
There was a problem hiding this comment.
When the remote PostHog flag changes to false during an active session and the host has closed the chat window, the contribution builder removes the Show/Stop commands and this guard also makes any cached Stop command a no-op. The remaining end-user stop control exists only inside the now-closed chat window, so the host cannot revoke the active share without closing its owner window or quitting the app. Gate session creation, but keep Show and Stop available for an already-active session.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
| guard socket?.send(.approve(user: user, role: role)) == .admitted else { | ||
| return | ||
| } | ||
| resolveAccessRequest(user: user, resolution: role == .editor ? .approvedEditor : .approvedViewer) |
There was a problem hiding this comment.
If the connection drops after socket.send admits an approval into the local mailbox but before URLSession sends it, this immediately marks the request as approved even though the Durable Object still considers the guest pending. There is no delivery completion or previous snapshot attached to this mutation, so the host sees a false permission decision until a later reconnect replays authoritative pending state; the deny path has the same issue. Keep the request pending until authoritative reconciliation or explicitly restore it when delivery fails.
AGENTS.md reference: AGENTS.md:L85-L85
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39cd18da3c
ℹ️ 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.markUnavailable(); | ||
| return; | ||
| } | ||
| this.scheduleReconnect(); |
There was a problem hiding this comment.
When a syntactically valid link refers to a session whose tombstone has already been deleted, the token route still successfully mints a token, while the Durable Object rejects the WebSocket upgrade with HTTP 404. Browsers surface that failed upgrade as an abnormal close (1006), so this unconditional reconnect path fetches another token and retries forever, leaving expired or mistyped links stuck on the connecting screen and repeatedly consuming the token rate limit. Add a bounded terminal condition or an authoritative existence signal that can transition the page to unavailable while preserving the short host-creation race retry.
Useful? React with 👍 / 👎.
| onTextChange: (input: string) => { | ||
| dispatch({ type: "setInput", input }); | ||
| notifyComposerTextChanged(input); |
There was a problem hiding this comment.
Every agent-composer text change now posts composer.changed across the WKWebView bridge, generates a native reply, and allocates a promise, but no production code assigns onComposerTextChanged, and the sharing protocol explicitly does not use composer edits. Consequently normal typing pays one unnecessary JS-to-native round trip per edit and can accumulate pending requests during rapid input. Install this callback only when a consumer exists, or debounce/batch the notifications before enabling them.
Useful? React with 👍 / 👎.
lawrencecchen
added
the
stale-revisit
2 participants
Summary
Boundaries
Verification
CmuxWorkspaceSharepackage tests and validates the Share Worker on pull requests.Design and operation details: multiplayer share guide and wire protocol.
Dictionary: A Durable Object is the single stateful relay instance for one invitation code. Delivery credit is the bounded unacknowledged data reserved for one socket. A dry-run bundles and validates a Worker deployment without publishing it.
Summary by CodeRabbit