Skip to content

Sidebar: hop selectedTabId row delivery to RunLoop.main + guard synchronous row onReceive #7341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gadgetsam wants to merge 2 commits into from
Closed

Sidebar: hop selectedTabId row delivery to RunLoop.main + guard synchronous row onReceive #7341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f72a2ee
Guard: ban synchronous onReceive delivery in sidebar row views
gadgetsam Jul 4, 2026
2d87b47
Sidebar: hop selectedTabId row delivery to RunLoop.main
gadgetsam Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/swift-file-length-budget.tsv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# Reduce counts as files shrink. CI fails if tracked files exceed this budget.
34499 CLI/cmux.swift
17954 Sources/AppDelegate.swift
16427 Sources/ContentView.swift
16434 Sources/ContentView.swift
14270 Sources/TerminalController.swift
13172 Sources/Workspace.swift
12348 cmuxTests/AppDelegateShortcutRoutingTests.swift
Expand Down
7 changes: 7 additions & 0 deletions Sources/ContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14068,6 +14068,13 @@ struct TabItemView: View, Equatable {
tabManager.selectedTabIdPublisher
.map { $0 == tab.id }
.removeDuplicates()
// The CurrentValueSubject replays synchronously on subscribe —
// which happens while the LazyVStack realizes this row, inside
// an in-flight layout transaction — and emits during willSet.
// Hop to RunLoop.main so the @State write below never lands in
// the transaction being laid out (#2586/#6556 livelock family);
// first render is covered by the live selectedTabId fallback.
.receive(on: RunLoop.main)
) { isSelected in
updateObservedActiveState(isSelected)
}
Expand Down
61 changes: 61 additions & 0 deletions scripts/check-sidebar-lazy-layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,56 @@
"inside a row is the #5323 feedback shape)"),
)

# `.onReceive(` in a row view. Combine delivery into a sidebar row must hop
# through `.receive(on:)`: the TabManager bridges are `CurrentValueSubject`s
# that replay the current value SYNCHRONOUSLY at subscribe time -- and a lazy
# row subscribes while the LazyVStack realizes it, inside an in-flight SwiftUI
# layout transaction -- and they emit during `willSet`, so a selection change
# made mid-update delivers mid-update. Either path lets the `onReceive` action
# write row `@State` inside the transaction being laid out: the same
# write-during-layout family as the #2586/#6556 livelocks (a row shipped this
# exact shape in stable v0.64.17 via `selectedTabIdPublisher`, observed
# livelocked in the wild on 2026-07-02/03). `NotificationCenter` publishers
# deliver synchronously on the posting thread and need the same hop.
ONRECEIVE_CALL = re.compile(r"\.onReceive\s*\(")

ROW_SYNC_ONRECEIVE_MESSAGE = (
".onReceive( without .receive(on:) in a row (synchronous publisher "
"delivery -- a CurrentValueSubject replays on subscribe while the "
"LazyVStack is realizing the row and emits during willSet -- writes row "
"@State inside the in-flight layout transaction, the #2586/#6556 "
"write-during-layout livelock family; route row subscriptions through "
".receive(on: RunLoop.main))"
)


def find_sync_onreceive(region):
"""Return True if ``region`` (neutralized Swift) contains an
``.onReceive(`` whose publisher argument lacks a ``.receive(on:`` hop.

The publisher expression is the balanced-parenthesis argument list of the
``.onReceive(`` call; the action trailing closure sits outside it, so a
``.receive(on:)`` inside the action cannot mask a synchronous publisher.
"""
for match in ONRECEIVE_CALL.finditer(region):
i = match.end() - 1 # at the opening '(' of the argument list
depth = 0
start = i
n = len(region)
while i < n:
ch = region[i]
if ch == "(":
depth += 1
elif ch == ")":
depth -= 1
if depth == 0:
break
i += 1
publisher_expr = re.sub(r"\s+", "", region[start:i + 1])
if ".receive(on:" not in publisher_expr:
return True
return False
Comment on lines +165 to +190

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unmatched parenthesis silently short-circuits the check

If region contains an .onReceive( whose opening ( is never balanced — for example because a macro or string interpolation produced a lone ( that the neutralizer didn't collapse — the while i < n loop exits with depth > 0 (without hitting the break). At that point i == n, so publisher_expr = re.sub(r"\s+", "", region[start:n+1]) is effectively the rest of the file from the ( onward. That's unlikely to contain .receive(on:, so the function would incorrectly return True and emit a false positive on otherwise clean code. A small guard like if i >= n: continue (skipping unbalanced matches) before the publisher_expr extraction would make the failure mode more predictable, though this scenario would only arise from malformed Swift that the compiler would also reject.


# Lazy-fill primitives the #6188 fix depends on. Each must remain present in the
# named function (after comments/strings are stripped).
REQUIRED_PRIMITIVES = (
Expand Down Expand Up @@ -476,6 +526,11 @@ def check_source(
"row-wrapper file contains forbidden per-row geometry "
"feedback: {0}".format(description)
)
if find_sync_onreceive(neutralized):
violations.append(
"row-wrapper file contains forbidden synchronous delivery: "
"{0}".format(ROW_SYNC_ONRECEIVE_MESSAGE)
)
Comment on lines +529 to +533

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 scan_all_rows branch of find_sync_onreceive has no meta-test coverage

Test cases (o) and (p) exercise find_sync_onreceive only through the type-body extraction path (a TabItemView struct is extracted and its body scanned). The scan_all_rows=True branch here — used for VerticalTabsSidebar+WorkspaceGroups.swift — calls find_sync_onreceive(neutralized) on the whole file, but no meta-test creates a wrapper-file fixture with a bare .onReceive( to verify that branch fires. Currently neither target file has any .onReceive calls, so the gap has no production impact today; if the wrapper file gains a synchronous subscription in future the guard would catch it, but only if this branch hasn't silently broken in the interim.

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!

for name in sorted(custom_layout_names):
if re.search(r"\b" + re.escape(name) + r"\b", neutralized):
violations.append(
Expand All @@ -501,6 +556,12 @@ def check_source(
"{0} contains forbidden per-row geometry feedback: "
"{1}".format(type_name, description)
)
if find_sync_onreceive(body):
violations.append(
"{0} contains forbidden synchronous delivery: {1}".format(
type_name, ROW_SYNC_ONRECEIVE_MESSAGE
)
)
for name in sorted(custom_layout_names):
if re.search(r"\b" + re.escape(name) + r"\b", body):
violations.append(
Expand Down
50 changes: 50 additions & 0 deletions tests/test_ci_sidebar_lazy_layout_guard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@
wild on 2026-07-02.
(l) Per-row `.anchorPreference` (the #5323 virtualization defeat) fails.
(m) A required row type missing from its file fails loudly (no silent skip).
(o) An `.onReceive(` in a row whose publisher lacks a `.receive(on:)` hop
fails — a CurrentValueSubject bridge replays synchronously while the
LazyVStack realizes the row (and emits during willSet), so the action
writes row @State inside the in-flight layout transaction. This exact
shape shipped in stable v0.64.17 via `selectedTabIdPublisher`.
(p) The same subscription routed through `.receive(on: RunLoop.main)`
passes, including when `.receive(on:)` spans multiple lines.
Comment on lines +27 to +33

@coderabbitai coderabbitai Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate case letter "(o)" reused for two different tests.

The new cases at lines 323-343 (and their docstring entries at lines 27-33) reuse label "(o)", which is already used later in the file (line 392: # (o) Whole-file row-wrapper scan...) for an unrelated pre-existing test. Two different test scenarios now share the same letter, making it harder to cross-reference a failing case back to the docstring.

Consider relettering the new sync/deferred onReceive cases (e.g., to the next unused letters) to avoid collision with the existing wrapper-scan case.

Also applies to: 323-343

🤖 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 `@tests/test_ci_sidebar_lazy_layout_guard.py` around lines 27 - 33, The new
onReceive test cases and their docstring entries reuse case label "(o)", which
collides with an existing unrelated case later in the file. Update the
identifiers for the new synchronous/deferred subscription tests in the test
class and matching docstring entries to the next unused letters so each scenario
has a unique cross-reference, keeping the labels in sync with the existing
whole-file row-wrapper scan case.

"""

import importlib.util
Expand Down Expand Up @@ -313,6 +320,49 @@ def row_fixture(row_body):
False, "per-row .anchorPreference (#5323 shape) fails",
) else 1

# (o) An .onReceive( whose publisher chain has no .receive(on:) hop:
# the CurrentValueSubject bridge replays synchronously during lazy row
# realization and emits during willSet, so the action's @State write
# lands inside the in-flight layout transaction (the #2586/#6556
# family). This shape shipped in stable v0.64.17 and livelocked in the
# wild on 2026-07-02/03. A .receive(on:) inside the ACTION closure
# (outside the publisher argument) must not mask the violation.
sync_onreceive_row = row_fixture(
" HStack { Text(tab.title) }\n"
" .onReceive(\n"
" tabManager.selectedTabIdPublisher\n"
" .map { $0 == tab.id }\n"
" .removeDuplicates()\n"
" ) { isSelected in\n"
" observedIsActive = isSelected\n"
" }"
)
failures += 0 if expect(
run_guard(write_fixture(workdir, "SyncOnReceiveRow.swift", sync_onreceive_row)),
False, "row .onReceive without .receive(on:) fails",
) else 1
Comment on lines +323 to +343

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test case (o) doesn't exercise its stated bypass scenario

The comment asserts that "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," but the fixture has no .receive(on:) in the action at all — it only tests the straightforward "no hop anywhere" path. The balanced-paren extraction is what prevents the bypass, but that specific property is never actually exercised by this fixture. A supplementary sub-test that places .receive(on: RunLoop.main) inside the trailing closure body (e.g., Just(isSelected).receive(on: RunLoop.main).sink { … }) while keeping the publisher argument unhopped would verify the claim and protect it against future refactors of find_sync_onreceive.

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!

Comment on lines +323 to +343

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test comment promises masking-via-action coverage that isn't in the fixture

The inline comment says "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," but the sync_onreceive_row fixture's action body is observedIsActive = isSelected — no .receive(on:) in sight. The algorithm is correct by design (the trailing closure sits outside the balanced parentheses, so publisher_expr never includes it), but the stated property isn't actually exercised. A companion fixture with .receive(on:) only in the action would make this guarantee explicit and prevent a future algorithm refactor from silently regressing it.

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!


# (p) The same subscription with a .receive(on: RunLoop.main) hop in
# the publisher chain passes -- also with the hop split across lines,
# since the real call sites chain one operator per line.
Comment on lines +323 to +347

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test claim for action-closure masking is unverified

The case (o) comment states "A .receive(on:) inside the ACTION closure (outside the publisher argument) must not mask the violation," and the guard's docstring (find_sync_onreceive) makes the same guarantee. However, the sync_onreceive_row fixture has no .receive(on:) anywhere — not in the publisher, not in the action — so the fixture only proves the basic "missing hop fails" path and does not actually exercise the masking scenario. A future regression where the balanced-paren scan accidentally consumed the action closure would silently pass this test. Adding a second fixture that places .receive(on: RunLoop.main) inside the action body (but not the publisher) and asserts expect(..., False, ...) would close this gap.

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!

deferred_onreceive_row = row_fixture(
" HStack { Text(tab.title) }\n"
" .onReceive(\n"
" tabManager.selectedTabIdPublisher\n"
" .map { $0 == tab.id }\n"
" .removeDuplicates()\n"
" .receive(\n"
" on: RunLoop.main\n"
" )\n"
" ) { isSelected in\n"
" observedIsActive = isSelected\n"
" }"
)
failures += 0 if expect(
run_guard(write_fixture(workdir, "DeferredOnReceiveRow.swift", deferred_onreceive_row)),
True, "row .onReceive with .receive(on:) passes",
) else 1
Comment on lines +323 to +364

@greptile-apps greptile-apps Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 New cases (o)/(p) are inserted before the pre-existing case (n) in code order

In the file the execution sequence is (l) → (o) → (p) → (n), which breaks the alphabetical numbering that the rest of the test follows and that the module docstring lists. A reader tracing a failure number will find (n) after (o)/(p) in both the docstring and execution order but only if they know to look past them. Inserting the two new cases after (n) (or renaming them to follow the existing sequence) would preserve the invariant that the letter labels and the code order agree.

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!


# (n) --file on a row-view source (no container functions) must not
# emit false "could not locate func" violations; row scanning still
# applies. (Greptile P2 on #7221.)
Expand Down