Repository navigation
fix(cmux-next): Home's dock takes tabs once the tree reports its ids, not at the next connect (FC-1 follow-up) - #30826
Conversation
… ids (FC-1 follow-up) HomeDock's allow-tabs step needs the dock's screen and pane resource ids; with none in the tree yet it returned 'waits for resource ids' and nothing ran it again until the next connection, so Home's pinned dock took no tab for the whole launch. The test applies a tree without the ids, runs the step, then applies the ids and expects one column.update. A seam passes the request in (no behavior change). Red on the base.
… not at the next connect (FC-1 follow-up)
HomeDock's allow-tabs step (column.update {allow_tabs: true}) needs the
dock's screen and pane resource ids. When the tree had none yet, it
returned and only the next connection tried again, so Home's pinned dock
could take no tab for the whole launch: every new tab aimed at it was
refused (dock-column-pinned) or had to open beside it. The step now arms
one wait on the store's records (ObservationStream, no timer) and runs
again when the ids first show; a later step replaces the wait, and it
ends when Home's page is gone.
Regression test: HomeDockAllowTabsTests (red at the commit before).
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
|
|
Review (subagent, correctness first): SHIP at ddcf71c Read the diff, HomeDock.swift at the head, ObservationStream, HomeService+Workspace (connect path), WindowHomes and DaemonService (connection lifetime). I did not run a local build or local tests. Correctness (no defects found)
Non-blocking notes
Evidence
Repo rules: the perf claim: none |
There was a problem hiding this comment.
3 issues found across 2 files
You’re at about 90% of the monthly reviewed-line 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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift:206">
P2: This cancels the pending wait before the guard decides whether this step does anything. A connect that returns nil here, such as one with `fresh` false after the key is set, still drops a wait that was created with `fresh` true and would have sent the update. Move the cancellation after the guard, or only cancel when this call proceeds to the ids check.</violation>
<violation number="2" location="Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift:235">
P2: `WindowHomes.ensureDock` creates `HomeDock()` temporarily, so this weak capture can find `self == nil` after `ensure` returns and abandon the ID wait. Keep the dock alive until this task finishes.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextHomeEngineTests/HomeDockAllowTabsTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextHomeEngineTests/HomeDockAllowTabsTests.swift:41">
P3: This named `UserDefaults` suite is never cleaned up, so every test run leaves a persistent domain behind. Keep the suite name in a variable and add `defer { defaults.removePersistentDomain(forName: suite) }`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| workspace: @MainActor @Sendable () -> WorkspaceModel?, fresh: Bool) async throws -> String? { | ||
| private func allowTabs(store: DaemonStore, workspace: @escaping @MainActor @Sendable () -> WorkspaceModel?, fresh: Bool, | ||
| allow: @escaping AllowTabs) async throws -> String? { | ||
| // This step (a connect's, or the wait's own) supersedes a wait for the ids. |
There was a problem hiding this comment.
P2: This cancels the pending wait before the guard decides whether this step does anything. A connect that returns nil here, such as one with fresh false after the key is set, still drops a wait that was created with fresh true and would have sent the update. Move the cancellation after the guard, or only cancel when this call proceeds to the ids check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift, line 206:
<comment>This cancels the pending wait before the guard decides whether this step does anything. A connect that returns nil here, such as one with `fresh` false after the key is set, still drops a wait that was created with `fresh` true and would have sent the update. Move the cancellation after the guard, or only cancel when this call proceeds to the ids check.</comment>
<file context>
@@ -179,17 +181,31 @@ public final class HomeDock {
- workspace: @MainActor @Sendable () -> WorkspaceModel?, fresh: Bool) async throws -> String? {
+ private func allowTabs(store: DaemonStore, workspace: @escaping @MainActor @Sendable () -> WorkspaceModel?, fresh: Bool,
+ allow: @escaping AllowTabs) async throws -> String? {
+ // This step (a connect's, or the wait's own) supersedes a wait for the ids.
+ allowTabsWait?.cancel()
+ allowTabsWait = nil
</file context>
| allowTabsWait = Task { [weak self] in | ||
| let identified: @MainActor @Sendable () -> Bool? = { | ||
| guard let placed = workspace().flatMap(Self.pageTab(in:)) else { return false } | ||
| return placed.screen.resourceID != nil && placed.pane.resourceID != nil ? true : nil | ||
| } | ||
| guard await self?.shown(identified) == true, !Task.isCancelled, let self else { return } |
There was a problem hiding this comment.
P2: WindowHomes.ensureDock creates HomeDock() temporarily, so this weak capture can find self == nil after ensure returns and abandon the ID wait. Keep the dock alive until this task finishes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift, line 235:
<comment>`WindowHomes.ensureDock` creates `HomeDock()` temporarily, so this weak capture can find `self == nil` after `ensure` returns and abandon the ID wait. Keep the dock alive until this task finishes.</comment>
<file context>
@@ -199,15 +215,35 @@ public final class HomeDock {
+ allowTabsWait?.cancel()
+ // task-owner: `allowTabsWait`, replaced by the next step and ended when the ids come or the page goes;
+ // event-driven (Observation of the store's records), no timer.
+ allowTabsWait = Task { [weak self] in
+ let identified: @MainActor @Sendable () -> Bool? = {
+ guard let placed = workspace().flatMap(Self.pageTab(in:)) else { return false }
</file context>
| allowTabsWait = Task { [weak self] in | |
| let identified: @MainActor @Sendable () -> Bool? = { | |
| guard let placed = workspace().flatMap(Self.pageTab(in:)) else { return false } | |
| return placed.screen.resourceID != nil && placed.pane.resourceID != nil ? true : nil | |
| } | |
| guard await self?.shown(identified) == true, !Task.isCancelled, let self else { return } | |
| allowTabsWait = Task { [self] in | |
| let identified: @MainActor @Sendable () -> Bool? = { | |
| guard let placed = workspace().flatMap(Self.pageTab(in:)) else { return false } | |
| return placed.screen.resourceID != nil && placed.pane.resourceID != nil ? true : nil | |
| } | |
| guard await shown(identified) == true, !Task.isCancelled else { return } |
| let store = DaemonStore() | ||
| store.noteHandshake(DaemonIdentity(capabilities: [DaemonCapability.pinnedDock.rawValue], generation: "home-dock-allow-tabs")) | ||
| store.apply(snapshot: Self.tree(identified: false)) | ||
| let defaults = try #require(UserDefaults(suiteName: "home-dock-allow-tabs-\(UUID().uuidString)")) |
There was a problem hiding this comment.
P3: This named UserDefaults suite is never cleaned up, so every test run leaves a persistent domain behind. Keep the suite name in a variable and add defer { defaults.removePersistentDomain(forName: suite) }.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextHomeEngineTests/HomeDockAllowTabsTests.swift, line 41:
<comment>This named `UserDefaults` suite is never cleaned up, so every test run leaves a persistent domain behind. Keep the suite name in a variable and add `defer { defaults.removePersistentDomain(forName: suite) }`.</comment>
<file context>
@@ -0,0 +1,59 @@
+ let store = DaemonStore()
+ store.noteHandshake(DaemonIdentity(capabilities: [DaemonCapability.pinnedDock.rawValue], generation: "home-dock-allow-tabs"))
+ store.apply(snapshot: Self.tree(identified: false))
+ let defaults = try #require(UserDefaults(suiteName: "home-dock-allow-tabs-\(UUID().uuidString)"))
+ let dock = HomeDock(defaults: defaults)
+ let sent = Sent()
</file context>
| let defaults = try #require(UserDefaults(suiteName: "home-dock-allow-tabs-\(UUID().uuidString)")) | |
| let suite = "home-dock-allow-tabs-\(UUID().uuidString)" | |
| let defaults = try #require(UserDefaults(suiteName: suite)) | |
| defer { defaults.removePersistentDomain(forName: suite) } |
…'s page goes, no polling (#30826 review follow-ups) (#30888) * test(cmux-next): Home's dock must take tabs once the tree reports its ids (FC-1 follow-up) HomeDock's allow-tabs step needs the dock's screen and pane resource ids; with none in the tree yet it returned 'waits for resource ids' and nothing ran it again until the next connection, so Home's pinned dock took no tab for the whole launch. The test applies a tree without the ids, runs the step, then applies the ids and expects one column.update. A seam passes the request in (no behavior change). Red on the base. * fix(cmux-next): Home's dock takes tabs once the tree reports its ids, not at the next connect (FC-1 follow-up) HomeDock's allow-tabs step (column.update {allow_tabs: true}) needs the dock's screen and pane resource ids. When the tree had none yet, it returned and only the next connection tried again, so Home's pinned dock could take no tab for the whole launch: every new tab aimed at it was refused (dock-column-pinned) or had to open beside it. The step now arms one wait on the store's records (ObservationStream, no timer) and runs again when the ids first show; a later step replaces the wait, and it ends when Home's page is gone. Regression test: HomeDockAllowTabsTests (red at the commit before). * test(cmux-next): HomeDock's allow_tabs wait: replacement, end when Home's page goes, no polling (FC-1 follow-up review notes) Review notes on #30826: the test's Task.yield loop becomes a continuation the recorded request resumes; the test's UserDefaults suite is removed; two tests cover a later step replacing the wait (one request when the ids come) and the wait ending when Home's page is gone. HomeDock: a comment on why the wait holds the dock across its await (window Homes keep no HomeDock), and allowTabsWait is readable for the tests. No behavior change. --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
Summary
Home's dock now takes tabs as soon as the tree reports its ids. Before, it waited for the next connection, so the dock could take no tab for a whole launch.
FC-1 follow-up (fuzz catalog). HomeDock pins Home's dock in its dock move, then lets other tabs in with a separate
column.update {allow_tabs: true}(HomeDock.allowTabs). That request names the dock's screen and pane by their resource ids. When the tree had no ids for them yet, the step returned "home dock allow_tabs waits for resource ids", and only the next connection tried again. Until then the dock took no tab at all:dock-column-pinned, as FC-1's diagnostic job 461a988b39553e691602fe76 logged);Fix (
Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift): when the ids are missing, the step starts one wait on the store's records (ObservationStream, no timer, no retry loop). When the page's screen and pane first have their ids, the step runs again. For the store Home (HomeService.dock), a later step (a reconnect's) replaces the wait. A window Home makes a newHomeDockper connect (WindowHomes.ensureDock), so each connect with no ids leaves its own wait; each sends one idempotentallow_tabs: truewhen the ids come, then ends. The wait ends when Home's page is gone. The request is now passed in (AllowTabs), so a test can see it; the app still sendssetColumnAllowsTabs(true, ...).Related: #30503 (landed: the real refusal text), #30676 (a new terminal aimed at a dock that takes no tabs opens beside it).
Changelog
Testing
Regression test:
HomeDockAllowTabsTests(CmuxNextHomeEngineTests, no daemon). It applies a tree whose Home page is in a pinned dock that takes no tabs and has no screen or pane ids, then runs the step. It expects "waits for resource ids" and no request. Then it applies the same tree with the ids, and expects exactly onecolumn.updatefor the dock's screen and pane, and the "dock takes tabs" setting recorded.CMUX_TEST_REQUIRE_GUI=1: step 5e7e52fc0bb67363c299a114 failed with 3 issues:sent.requests.map(\.screen) → [] == [scr_home], the same for the pane, and the setting not recorded.CMUX_TEST_REQUIRE_GUI=1: step c42815e311f01f1104ed86c8 passedHomeDockAllowTabsTests(1 of 1) and the existingHomeDockOpensTests(10 of 10).cmux-ci compile-check --app --package CmuxNext --recipe-version 2job 1342e802aaafd68c920b1d2b passed at ddcf71c (CmuxNext, app, cmux-tui).Observed / Not run
prepush-gate.pywith no new violations.HomeDockstrongly across its await;removePersistentDomainfor the test's UserDefaults suite; a continuation signal instead of theTask.yieldloop; tests for the wait's replacement and its end when the page goes.allow_tabsthen applied (FC-1's diagnostic log had no HomeDock step lines); the full CmuxNext suite; a dogfood build.No capture: no visible change beyond Home's dock accepting tabs as it already should.
perf claim: none