Repository navigation
Conversation
|
@m2na7 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughAdds per-panel retention for remote port scans so transient empty TTY attribution does not immediately clear previously detected ports, and adds tests for the retention limit and reset paths. ChangesPort Scan Retention
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PortScanTask
participant RemoteSessionCoordinator
participant RemoteSessionHost
PortScanTask->>RemoteSessionCoordinator: performRemotePortScanLocked()
RemoteSessionCoordinator->>RemoteSessionCoordinator: scan ports per panel from TTY attribution
RemoteSessionCoordinator->>RemoteSessionCoordinator: retainingTransientEmptyScansLocked(results)
alt empty scan under retention limit
RemoteSessionCoordinator->>RemoteSessionCoordinator: keep previous ports and increment miss count
else empty scan over retention limit
RemoteSessionCoordinator->>RemoteSessionCoordinator: clear ports for panel
else non-empty scan
RemoteSessionCoordinator->>RemoteSessionCoordinator: reset miss count and update ports
end
RemoteSessionCoordinator->>RemoteSessionHost: publish reconciled ports snapshot
Related issues: Suggested reviewers: N/A 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
|
✅ Action performedReview finished.
|
@m2na7 I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryThis PR fixes a sidebar port-badge flicker in remote SSH workspaces by retaining last-known ports across bounded consecutive empty TTY scans. The TTY-scoped attribution races under load and can return an empty-but-successful scan for a still-listening port, causing the badge to disappear and shift surrounding rows; the fix reconciles each fresh scan against the previous state instead of overwriting it wholesale.
Confidence Score: 5/5Safe to merge. The change is tightly scoped to the TTY-scoped scan path and all four teardown paths now correctly clear both companion dictionaries in lockstep. The reconciliation logic in No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[performRemotePortScanLocked] --> B{ttyNames empty?}
B -- yes --> C[clear both dicts, publish empty]
B -- no --> D[scanRemotePortsByPanelLocked]
D -- throws --> E[log error, leave state intact]
D -- scanned --> F[retainingTransientEmptyScansLocked]
F --> G{ports non-empty?}
G -- yes --> H[adopt fresh ports]
G -- no --> I{previous non-empty AND missCount <= limit?}
I -- yes --> J[retain previous, increment miss]
I -- no --> K[omit panel, evict miss]
H --> L[remotePortScanEmptyMissByPanel = misses]
J --> L
K --> L
L --> M[remoteScannedPortsByPanel = reconciled]
M --> N[publishPortsSnapshotLocked]
%%{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"}}}%%
flowchart TD
A[performRemotePortScanLocked] --> B{ttyNames empty?}
B -- yes --> C[clear both dicts, publish empty]
B -- no --> D[scanRemotePortsByPanelLocked]
D -- throws --> E[log error, leave state intact]
D -- scanned --> F[retainingTransientEmptyScansLocked]
F --> G{ports non-empty?}
G -- yes --> H[adopt fresh ports]
G -- no --> I{previous non-empty AND missCount <= limit?}
I -- yes --> J[retain previous, increment miss]
I -- no --> K[omit panel, evict miss]
H --> L[remotePortScanEmptyMissByPanel = misses]
J --> L
K --> L
L --> M[remoteScannedPortsByPanel = reconciled]
M --> N[publishPortsSnapshotLocked]
Reviews (5): Last reviewed commit: "Clear the port-scan miss counter on ever..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func retainingTransientEmptyScansLocked(_ scanned: [UUID: [Int]]) -> [UUID: [Int]] { | ||
| var reconciled: [UUID: [Int]] = [:] | ||
| var misses: [UUID: Int] = [:] | ||
| for (panelId, ports) in scanned { | ||
| if !ports.isEmpty { | ||
| reconciled[panelId] = ports | ||
| continue | ||
| } | ||
| let previous = remoteScannedPortsByPanel[panelId] ?? [] | ||
| let missCount = (remotePortScanEmptyMissByPanel[panelId] ?? 0) + 1 | ||
| guard !previous.isEmpty, missCount <= Self.remotePortScanEmptyRetentionLimit else { continue } | ||
| reconciled[panelId] = previous | ||
| misses[panelId] = missCount | ||
| } | ||
| remotePortScanEmptyMissByPanel = misses | ||
| return reconciled |
There was a problem hiding this comment.
remotePortScanEmptyMissByPanel is a new retention counter that makes remoteScannedPortsByPanel deliberately stale for up to remotePortScanEmptyRetentionLimit cycles — the invariant "published ports reflect the latest scan" is now intentionally broken. Per the architectural rule this is a timing repair that patches the symptom (flickering badge) by adding a new side-channel cache, while the TTY attribution race in remotePortScanScript (child process fd/0 not matching the shell TTY under load) remains in place as the root cause. The fix is bounded and the constant is named, which mitigates the risk, but future readers maintaining the scan logic will need to account for the fact that remoteScannedPortsByPanel is no longer a faithful view of the last scan. Consider leaving an explicit invariant comment on both state fields noting that remoteScannedPortsByPanel may lag the last physical scan by up to remotePortScanEmptyRetentionLimit cycles, and that remotePortScanEmptyMissByPanel must be kept consistent with it at every mutation site.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
Greptile SummaryThis PR fixes sidebar port-badge flickering in remote SSH workspaces by changing the TTY-scoped port scan from a wholesale-replace to a reconcile-and-retain strategy: panels that report an empty scan keep their last-known ports for up to
Confidence Score: 4/5Safe to merge; the reconcile logic is correct and the new tests deterministically cover the regression and the drop-after-limit case. The reconcile logic, retention counter lifecycle, and test coverage are all solid. The only gap is that remotePortScanEmptyMissByPanel is not cleared in stopAllLocked and the proxy-error branch of handleProxyBrokerUpdateLocked, making the teardown contract inconsistent with suspendRemotePortScanningLocked and the empty-TTY path — though the previous.isEmpty guard in the reconcile loop prevents any observable misbehavior from this inconsistency. RemoteSessionCoordinator.swift (stopAllLocked) and RemoteSessionCoordinator+PortScan.swift (handleProxyBrokerUpdateLocked error branch) — both are missing remotePortScanEmptyMissByPanel.removeAll() next to the existing remoteScannedPortsByPanel.removeAll() calls. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant BurstTask as Burst Task
participant Queue as coordinator.queue
participant Scan as performRemotePortScanLocked
participant Reconcile as retainingTransientEmptyScansLocked
participant Host as RemoteSessionHosting
BurstTask->>Queue: remotePortScanBurstStepElapsed
Queue->>Scan: performRemotePortScanLocked()
Scan->>Scan: scanRemotePortsByPanelLocked(ttyNamesByPanel)
Note over Scan: SSH exec → parse TTY:port pairs
Scan->>Reconcile: retainingTransientEmptyScansLocked(scanned)
alt panel scanned non-empty
Reconcile->>Reconcile: "reconciled[panelId] = freshPorts"
Reconcile->>Reconcile: missCount reset (not in misses)
else panel scanned empty
Reconcile->>Reconcile: "previous = remoteScannedPortsByPanel[panelId]"
Reconcile->>Reconcile: "missCount = (emptyMissByPanel[panelId] ?? 0) + 1"
alt "previous non-empty AND missCount <= retentionLimit (2)"
Reconcile->>Reconcile: "reconciled[panelId] = previous (retained)"
Reconcile->>Reconcile: "misses[panelId] = missCount"
else previous empty OR missCount exceeded
Reconcile->>Reconcile: continue (port cleared)
end
end
Reconcile->>Reconcile: "remotePortScanEmptyMissByPanel = misses"
Reconcile-->>Scan: reconciled map
Scan->>Scan: "remoteScannedPortsByPanel = reconciled"
Scan->>Host: publishPortsSnapshotLocked()
%%{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 BurstTask as Burst Task
participant Queue as coordinator.queue
participant Scan as performRemotePortScanLocked
participant Reconcile as retainingTransientEmptyScansLocked
participant Host as RemoteSessionHosting
BurstTask->>Queue: remotePortScanBurstStepElapsed
Queue->>Scan: performRemotePortScanLocked()
Scan->>Scan: scanRemotePortsByPanelLocked(ttyNamesByPanel)
Note over Scan: SSH exec → parse TTY:port pairs
Scan->>Reconcile: retainingTransientEmptyScansLocked(scanned)
alt panel scanned non-empty
Reconcile->>Reconcile: "reconciled[panelId] = freshPorts"
Reconcile->>Reconcile: missCount reset (not in misses)
else panel scanned empty
Reconcile->>Reconcile: "previous = remoteScannedPortsByPanel[panelId]"
Reconcile->>Reconcile: "missCount = (emptyMissByPanel[panelId] ?? 0) + 1"
alt "previous non-empty AND missCount <= retentionLimit (2)"
Reconcile->>Reconcile: "reconciled[panelId] = previous (retained)"
Reconcile->>Reconcile: "misses[panelId] = missCount"
else previous empty OR missCount exceeded
Reconcile->>Reconcile: continue (port cleared)
end
end
Reconcile->>Reconcile: "remotePortScanEmptyMissByPanel = misses"
Reconcile-->>Scan: reconciled map
Scan->>Scan: "remoteScannedPortsByPanel = reconciled"
Scan->>Host: publishPortsSnapshotLocked()
|
Greptile SummaryThis PR fixes a sidebar port-badge flicker on remote SSH workspaces by retaining a panel's last-known ports across a bounded number of consecutive empty scans, rather than immediately dropping them on the first empty-but-successful attribution miss. The change is confined to the TTY-scoped scan path (
Confidence Score: 4/5The port-scan retention logic is sound and the new tests deterministically cover both the flicker fix and the sustained-drop case; two teardown paths that reset the scanned-ports map do not also reset the miss-count map, which is inconsistent but guarded by the existing !previous.isEmpty check. The reconciliation loop in retainingTransientEmptyScansLocked is correct, and both suspendRemotePortScanningLocked and the empty-TTY branch of performRemotePortScanLocked clear the miss counter correctly. However, stopAllLocked and the proxy-error branch of handleProxyBrokerUpdateLocked each tear down remoteScannedPortsByPanel without clearing remotePortScanEmptyMissByPanel. A future change that consults the miss counter without first verifying the scanned-ports map could produce spurious retention after a stop or a proxy error cycle. Both teardown sites in RemoteSessionCoordinator.swift — stopAllLocked and the .error case of handleProxyBrokerUpdateLocked — need remotePortScanEmptyMissByPanel.removeAll() added alongside the existing remoteScannedPortsByPanel.removeAll() calls. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[performRemotePortScanLocked] --> B{ttyNamesByPanel empty?}
B -- Yes --> C[removeAll: scanned + misses\npublishPortsSnapshotLocked]
B -- No --> D[scanRemotePortsByPanelLocked]
D --> E{ssh throws?}
E -- Yes --> F[keep previous state\nlog error]
E -- No --> G[retainingTransientEmptyScansLocked]
G --> H{ports non-empty\nfor panel?}
H -- Yes --> I[adopt fresh ports\nmiss count implicitly reset]
H -- No --> J{previous non-empty AND missCount le 2?}
J -- Yes --> K[retain previous ports\nincrement miss count]
J -- No --> L[drop port\nmiss count not stored]
I --> M[remoteScannedPortsByPanel = reconciled]
K --> M
L --> M
M --> N[publishPortsSnapshotLocked]
%%{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"}}}%%
flowchart TD
A[performRemotePortScanLocked] --> B{ttyNamesByPanel empty?}
B -- Yes --> C[removeAll: scanned + misses\npublishPortsSnapshotLocked]
B -- No --> D[scanRemotePortsByPanelLocked]
D --> E{ssh throws?}
E -- Yes --> F[keep previous state\nlog error]
E -- No --> G[retainingTransientEmptyScansLocked]
G --> H{ports non-empty\nfor panel?}
H -- Yes --> I[adopt fresh ports\nmiss count implicitly reset]
H -- No --> J{previous non-empty AND missCount le 2?}
J -- Yes --> K[retain previous ports\nincrement miss count]
J -- No --> L[drop port\nmiss count not stored]
I --> M[remoteScannedPortsByPanel = reconciled]
K --> M
L --> M
M --> N[publishPortsSnapshotLocked]
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…the retention invariant
|
Pushed a18ee88 addressing the review feedback:
@coderabbitai review |
@m2na7 I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 7245 --repo manaflow-ai/cmux | head -400Length of output: 17852 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 7245 --repo manaflow-ai/cmux | wc -l
gh pr diff 7245 --repo manaflow-ai/cmux | tail -80Length of output: 3374 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp 2>/dev/null || true
gh repo clone manaflow-ai/cmux -- --depth 50 2>/dev/null || trueLength of output: 261 🏁 Script executed: #!/bin/bash
set -euo pipefail
git clone --depth 50 https://github.com/manaflow-ai/cmux.git repo 2>&1 | tail -5
cd repo
git fetch origin pull/7245/head:pr7245 --depth 50 2>&1 | tail -5
git checkout pr7245 2>&1 | tail -5
sed -n '1,40p' Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swift | head -5
grep -n "func stopAllLocked" -n Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swift
sed -n '440,480p' Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftLength of output: 4305 🏁 Script executed: #!/bin/bash
cd repo
sed -n '245,270p' Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftLength of output: 1186 Verified. The follow-up commit does exactly what's described: The rest of the retention logic ( This looks good to merge from my side. [approve] |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for this! Remote port badges now retain bounded transient scan misses in a023bef. You had this first :) |
This branch was successfully deployed
2 participants
Fixes #7244.
Summary
:3001) flickers — it disappears and reappears every few seconds — and because the ports row only occupies height when the port set is non-empty, every disappearance collapses the row and shifts the sibling rows down/up.The bug
With a remote SSH workspace running a dev server (a listening port under the session shell), the sidebar port badge flickers:
:3001shows, vanishes, shows again, repeatedly. The ports row is only laid out when the port list is non-empty (Sources/ContentView.swift, the// Ports rowblock gated on!listeningPorts.isEmpty), so each flicker removes and re-adds the row and its spacing, visibly shifting every row beneath it. The flicker is worst while the workspace is actively working (the agent is spawning subprocesses) and settles when activity calms.Root cause
RemoteSessionCoordinatordiscovers listening ports per panel by attributing each listening socket to a tracked TTY through the emitting process's controlling terminal (readlink /proc/<pid>/fd/0, with anlsoffallback keyed byps -t) — seeremotePortScanScriptinRemoteSessionCoordinator+PortScan.swift. That attribution is best-effort and races: a dev server frequently listens from a child process whosefd/0is not the shell's TTY (redirected, detached, or reparented), andss'spid=visibility and the process tree churn under load. So a scan can succeed yet report no port for a panel whose server is still listening.performRemotePortScanLockedthen replaces the per-panel port map wholesale with that scan result:publishPortsSnapshotLockedpublishes the union ofremoteScannedPortsByPanelandpolledRemotePorts, so when the scan comes back empty the published set is empty and the badge disappears until the next scan re-attributes the port. Notably, only an ssh failure (thecatch) preserves the previous ports; an empty success clears them. The scan bursts on shell activity and polls every few seconds, so any single empty pass is republished immediately as a flicker.The fix
Reconcile a fresh scan against the last-known ports instead of overwriting: a panel that scans non-empty adopts the fresh ports; a panel that scans empty keeps its previous ports until it has been empty more than
remotePortScanEmptyRetentionLimit(2) scans in a row, then clears. This absorbs the attribution's transient misses (no flicker) while a port that stays gone past the limit still clears within a couple of scan cycles. The retention counter is cleared alongside the existing port teardown (scanner suspend and the empty-TTY branch), so a disabled or re-armed scanner starts clean.The change is confined to the TTY-scoped scan path; the host-wide/delta poll modes, the settings gating (#6136), and connection-establishing ssh are untouched.
Testing
swift test --package-path Packages/macOS/CmuxRemoteSession— 42 tests pass.New
RemotePortScanRetentionTestsdrives the queue-confinedperformRemotePortScanLockedthrough a scripted process runner (scripts the ssh scan stdout:"ttys010\t3001\n", then"") and a recording host that captures the published port snapshot — fully deterministic, no wall-clock waits:3001followed by one empty scan keeps3001(the flicker regression);The same regression test fails before the reconciliation and passes after it:
Before (wholesale replace — one empty scan drops the still-listening port):
After (reconcile retains the port across the transient empty scan):
A demo video isn't included because the flicker is a timing-dependent detection race on a remote host rather than a deterministic UI action; the two tests reproduce the drop and its fix deterministically instead.
Scope notes
remotePortScanEmptyRetentionLimit = 2trades up to ~2 extra scan cycles of a stale badge after a server genuinely stops for the elimination of the flicker. It is a named constant so the trade-off is easy to tune.Review Trigger (Copy/Paste as PR comment)
Checklist
swift test --package-path Packages/macOS/CmuxRemoteSession, 42 tests)RemotePortScanRetentionTests)swift-file-length-budget.tsvfor the two touched files)Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stops the sidebar port badge from flickering in remote SSH workspaces by keeping last-known ports across transient empty TTY scans. Ports that are truly gone still clear after a short threshold.
RemotePortScanRetentionTeststo cover teardown clearing and transient-miss retention.Written for commit a18ee88. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests