Repository navigation
test(cmux-next): HomeDock allow_tabs wait: replacement, end when Home's page goes, no polling (#30826 review follow-ups) - #30888
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).
…me'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.
…e-dock-allow-tabs-followups # Conflicts: # Packages/macOS/CmuxNext/Sources/CmuxNextHomeEngine/HomeDock.swift # Packages/macOS/CmuxNext/Tests/CmuxNextHomeEngineTests/HomeDockAllowTabsTests.swift
There was a problem hiding this comment.
1 issue found across 2 files
You’re at about 92% 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:241">
P2: The resource-ID wait can disappear for window Homes before it observes the IDs. `WindowHomes.ensureDock` creates `HomeDock()` as a temporary, while this task captures `self` weakly; after `ensure` returns, the task can start with `self == nil` and never send `allow_tabs`. Retain the dock for the run or capture it strongly with cancellation-safe cleanup instead of relying on this weak capture.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| return placed.screen.resourceID != nil && placed.pane.resourceID != nil ? true : nil | ||
| } | ||
| // `self?.shown` holds this HomeDock across the await on purpose: a window Home makes a HomeDock per connect | ||
| // (`WindowHomes.ensureDock`) and keeps none, so the wait is what keeps it until the ids come or the page goes. |
There was a problem hiding this comment.
P2: The resource-ID wait can disappear for window Homes before it observes the IDs. WindowHomes.ensureDock creates HomeDock() as a temporary, while this task captures self weakly; after ensure returns, the task can start with self == nil and never send allow_tabs. Retain the dock for the run or capture it strongly with cancellation-safe cleanup instead of relying on this weak capture.
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 241:
<comment>The resource-ID wait can disappear for window Homes before it observes the IDs. `WindowHomes.ensureDock` creates `HomeDock()` as a temporary, while this task captures `self` weakly; after `ensure` returns, the task can start with `self == nil` and never send `allow_tabs`. Retain the dock for the run or capture it strongly with cancellation-safe cleanup instead of relying on this weak capture.</comment>
<file context>
@@ -237,6 +237,8 @@ public final class HomeDock {
return placed.screen.resourceID != nil && placed.pane.resourceID != nil ? true : nil
}
+ // `self?.shown` holds this HomeDock across the await on purpose: a window Home makes a HomeDock per connect
+ // (`WindowHomes.ensureDock`) and keeps none, so the wait is what keeps it until the ids come or the page goes.
guard await self?.shown(identified) == true, !Task.isCancelled, let self else { return }
allowTabsWait = nil // the step below must not cancel the task it runs in
</file context>
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 0a4c836 to compare; do not merge until green; see lane-rules Shipped in:
|
|
Found 4 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Review follow-ups to #30826 (Home's dock takes tabs once the tree reports its ids). This PR changes tests and one comment only, with no behavior change.
From the independent review of #30826 (#30826 (comment)):
HomeDockAllowTabsTestsno longer loops 2000Task.yieldcalls (REVIEW.md section 6). The recorded request resumes a continuation (Sent.next(after:)), and the test also awaits the wait task itself. The suite's time limit bounds a request that never comes.removePersistentDomain), so no plist is left on the test host.aLaterStepReplacesTheWait: two steps with no ids, then the ids. Exactly onecolumn.updategoes, and the first wait ends with none.theWaitEndsWhenHomesPageIsGone: the wait ends when Home's page leaves the tree, and the ids arriving later send nothing.HomeDock:HomeDockacross its await. A window Home makes aHomeDockper connect and keeps none, so a later "fix" to a weak capture would break it.allowTabsWaitisprivate(set)so the tests can await it.Changelog
none
Testing
These are new and changed tests. They cover behavior #30826 already added, so there is no red commit. They pass at the base of this change: the HomeDock behavior is #30826's, unchanged here.
cmux-ci compile-check --app --package CmuxNext --recipe-version 2job 6c98bb17a942958e881d7ac8 passed at 43924e7 (before the merge of feat). Job bf17cd1e372f688d77a98f09 runs at the head f842b96.CMUX_TEST_REQUIRE_GUI=1,HomeDockAllowTabsTests: GREEN_RESULTObserved / Not run
prepush-gate.pywith no new violations.perf claim: none
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Review follow-ups to #30826; tests and one comment only, with no behavior change.
HomeDockAllowTabsTestsno longer loops 2000Task.yieldcalls — a recorded request resumes a continuation, and each test awaits the wait task itself.UserDefaultssuite, so no plist is left on the test host.allow_tabsrequest when the ids come), and the wait ends when Home's page leaves the tree.HomeDock.allowTabsWaitis nowprivate(set)for the tests, and a comment explains why the wait holds theHomeDockacross its await.Written for commit f842b96. Summary will update on new commits.