Repository navigation
TEMP vendor lint eval: architecture and concurrent - #3495
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds ChangesFocus Restoration Coordinator & UI Panel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 25 seconds.Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR adds a single Swift fixture,
Confidence Score: 2/5Do not merge — five independent P1 violations across actor isolation, blocking primitives, concurrency, and state architecture. Five separate P1 findings across architectural symptom patching, two blocking/timing primitives (asyncAfter + Task.sleep), an untracked Task, a missing @Concurrent annotation, and legacy ObservableObject/StateObject. Per the confidence guidance, multiple P1s pull below the 4/5 P1 ceiling; score is 2/5. Sources/ReviewBotVendorArchitectureEval.swift — every major block in this file contains a violation. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["restoreFocus(workspaceID:surfaceID:payload:)"] --> B["① Immediate write\npendingFocusSurfaceID = surfaceID"]
A --> C["② asyncAfter +0.3s\npendingFocusSurfaceID = lastKnownSurfaceID"]
A --> D["③ Untracked Task\nawait decodeLargeWorkspacePayload"]
D --> E{"decoded.contains(surfaceID)"}
E -- yes --> F["Task.sleep 150ms\npendingFocusSurfaceID = surfaceID"]
E -- no --> G["Task ends (no write)"]
B & C & F --> H[("pendingFocusSurfaceID\n← three unordered writers\nrace to win")]
subgraph "Missing @concurrent"
D -->|"runs on @MainActor\nnot off it"| D2["nonisolated async decodeLargeWorkspacePayload\n(no @concurrent)"]
end
style H fill:#f88,stroke:#c33
style C fill:#fdb,stroke:#a60
style F fill:#fdb,stroke:#a60
style D2 fill:#fdb,stroke:#a60
Reviews (1): Last reviewed commit: "Add architecture vendor lint eval fixtur..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static var lastKnownSurfaceID: UUID? | ||
| private var didScheduleRepair = false | ||
|
|
||
| func restoreFocus(workspaceID: UUID, surfaceID: UUID, payload: Data) { | ||
| activeWorkspaceID = workspaceID | ||
| pendingFocusSurfaceID = surfaceID | ||
| Self.lastKnownSurfaceID = surfaceID | ||
|
|
||
| if !didScheduleRepair { | ||
| didScheduleRepair = true | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { | ||
| self.pendingFocusSurfaceID = Self.lastKnownSurfaceID | ||
| } | ||
| } | ||
|
|
||
| Task { | ||
| let decoded = await decodeLargeWorkspacePayload(payload) | ||
| if decoded.contains(surfaceID.uuidString) { | ||
| try? await Task.sleep(nanoseconds: 150_000_000) | ||
| self.pendingFocusSurfaceID = surfaceID | ||
| } | ||
| } |
There was a problem hiding this comment.
pendingFocusSurfaceID
restoreFocus creates three independent, unordered writers for the same pendingFocusSurfaceID field: an immediate assignment (line 14), a DispatchQueue.main.asyncAfter block (lines 18–21), and an untracked Task (lines 23–29). On top of that, lastKnownSurfaceID is a static var acting as a second owner of the same surface identity. didScheduleRepair is a flag caching a scheduling decision that should not exist. None of these name an invariant or a single source of truth — whichever writer fires last wins, producing a race that is impossible to reason about across multiple restoreFocus calls. The root cause is that focus state has no explicit model: the invariant ("which surface should own focus and why") belongs in a single actor-isolated store with a defined state machine, not distributed across a flag, a static, and competing async closures.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
| didScheduleRepair = true | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { | ||
| self.pendingFocusSurfaceID = Self.lastKnownSurfaceID | ||
| } |
There was a problem hiding this comment.
DispatchQueue.main.asyncAfter timing repair instead of a real signal
asyncAfter(deadline: .now() + 0.3) is a timing-based patch over a focus-lifecycle race, not a response to a real event. Per the cmux blocking-runtime rule, asyncAfter in a focus path fails CI by default. The 0.3s constant cannot be the correct delay on every device/load, and there is no cancellation if restoreFocus is called again before the block fires. The right replacement is a real lifecycle callback, state-change notification, or actor message that signals when the surface is actually ready to receive focus.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| if decoded.contains(surfaceID.uuidString) { | ||
| try? await Task.sleep(nanoseconds: 150_000_000) |
There was a problem hiding this comment.
Task.sleep in production focus path
try? await Task.sleep(nanoseconds: 150_000_000) (150 ms) is a timing-based synchronization primitive in production code inside a focus path — a category the cmux blocking-runtime rule explicitly calls out as latency-sensitive. Silencing the error with try? also suppresses cancellation, meaning the sleep cannot be interrupted if the view disappears or restoreFocus fires again. The real event to wait on is whatever condition makes decoded.contains(surfaceID.uuidString) meaningful — that readiness signal should drive the state update, not a fixed delay.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
|
|
||
| Task { | ||
| let decoded = await decodeLargeWorkspacePayload(payload) | ||
| if decoded.contains(surfaceID.uuidString) { | ||
| try? await Task.sleep(nanoseconds: 150_000_000) | ||
| self.pendingFocusSurfaceID = surfaceID | ||
| } |
There was a problem hiding this comment.
Task with meaningful state lifecycle
The Task { ... } spawned inside restoreFocus writes pendingFocusSurfaceID but is never stored, cancelled, or tied to any caller-owned scope. Per the cmux concurrency-modernization rule, a fire-and-forget Task with meaningful lifecycle is a failure. If the view is dismissed or restoreFocus is called again, the dangling Task will still complete and overwrite state. The Task handle should be stored in a @MainActor-isolated property and cancelled on the next restoreFocus call or on view teardown (e.g., .task {} modifier, which is automatically cancelled on disappear).
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
|
|
||
| nonisolated func decodeLargeWorkspacePayload(_ data: Data) async -> String { |
There was a problem hiding this comment.
@concurrent on CPU-bound nonisolated async decoder
Under Swift 6's NonisolatedNonsendingByDefault behavior (SE-0461), a nonisolated async function without @concurrent does not automatically hop off the caller's actor — it inherits the caller's isolation. decodeLargeWorkspacePayload is called from inside the @MainActor-isolated Task block, so String(decoding:as:) runs on the main thread despite the nonisolated annotation. For a function intended to do potentially large UTF-8 decoding off the main actor, @concurrent is required to actually establish the actor boundary.
Rule Used: Flag incorrect or missing use of Swift @Concurrent... (source)
| @MainActor | ||
| final class ReviewBotVendorFocusCoordinator: ObservableObject { | ||
| @Published var activeWorkspaceID: UUID? | ||
| @Published var pendingFocusSurfaceID: UUID? |
There was a problem hiding this comment.
ObservableObject/@Published/@StateObject — use @Observable + @State
ReviewBotVendorFocusCoordinator is new cmux-owned state introduced by this PR and uses the legacy ObservableObject/@Published Combine stack, consumed via @StateObject. Per the cmux swiftui-state-layout rule, new view state should use the @Observable macro with @State instead. ObservableObject publishes the entire object on any @Published change, causing broader invalidation than @Observable's per-property tracking.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (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!
| let payload: Data | ||
|
|
||
| var body: some View { | ||
| Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing") |
There was a problem hiding this comment.
Text fallback string
"missing" is passed as a plain String to Text, bypassing SwiftUI's LocalizedStringKey lookup. If the surface ID is absent, users see a raw English literal. Use a localized string key or a LocalizedStringResource.
| Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing") | |
| Text(coordinator.pendingFocusSurfaceID?.uuidString ?? String(localized: "missing")) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/ReviewBotVendorArchitectureEval.swift (1)
5-7: ⚡ Quick winMigrate from
ObservableObject/@Publishedto@Observable.This uses the legacy
ObservableObject+@Publishedpattern. The codebase should use the@Observablemacro (Observation framework, available since iOS 17 / macOS 14), which removes the@Publishedboilerplate, eliminates gratuitous republishing of unobserved properties, and is the expected model for new SwiftUI stores.♻️ Proposed refactor
-@MainActor -final class ReviewBotVendorFocusCoordinator: ObservableObject { - `@Published` var activeWorkspaceID: UUID? - `@Published` var pendingFocusSurfaceID: UUID? +@MainActor +@Observable +final class ReviewBotVendorFocusCoordinator { + var activeWorkspaceID: UUID? + var pendingFocusSurfaceID: UUID?The call site would change from
@StateObjectto@State:- `@StateObject` private var coordinator = ReviewBotVendorFocusCoordinator() + `@State` private var coordinator = ReviewBotVendorFocusCoordinator()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ReviewBotVendorArchitectureEval.swift` around lines 5 - 7, Replace the ObservableObject/@Published pattern in ReviewBotVendorFocusCoordinator with the Observation macro: annotate the type with `@Observable` instead of inheriting ObservableObject and remove `@Published` from activeWorkspaceID and pendingFocusSurfaceID; update any view call sites that used `@StateObject` var coordinator to use `@State` (or the appropriate observation-backed property wrapper) so the coordinator is created and observed via the Observation framework.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ReviewBotVendorArchitectureEval.swift`:
- Line 45: The Text view currently uses an unlocalized fallback literal
("missing") in the expression Text(coordinator.pendingFocusSurfaceID?.uuidString
?? "missing"); replace this literal with a localized string using
String(localized: "reviewbot.pendingFocus.missing", defaultValue: "missing") and
add the "reviewbot.pendingFocus.missing" key with English and Japanese entries
to Resources/Localizable.xcstrings so the UI string is localized.
- Around line 44-52: The view is calling coordinator.restoreFocus from .onAppear
which mutates `@Published` state and schedules async work; move this side-effect
out of the view-body by either passing the needed inputs into the coordinator at
construction or invoking restoreFocus from a SwiftUI lifecycle modifier that is
safe for state mutation (e.g. use .task(id: workspaceID, surfaceID, payload) or
call restoreFocus from the parent when creating the coordinator), so locate the
call to restoreFocus (and the Text view using coordinator.pendingFocusSurfaceID)
and replace the .onAppear invocation with a construction-time call or a
.task(id:) invocation keyed to workspaceID/surfaceID/payload to prevent state
mutation during body computation.
- Around line 17-30: The current code spawns an untracked Task and uses
DispatchQueue.main.asyncAfter while never resetting didScheduleRepair, causing
racing writes to pendingFocusSurfaceID; replace both paths with a single
cancellable Task stored on the instance (e.g., a property like
pendingFocusRepairTask) that you cancel on re-entry of restoreFocus, reset or
remove the didScheduleRepair flag logic, and perform a single cooperative
sequence inside that Task which awaits decodeLargeWorkspacePayload(payload) (or
sleeps for the fallback delay) and then sets pendingFocusSurfaceID (using
Self.lastKnownSurfaceID as the fallback) so only this Task owns writes to
pendingFocusSurfaceID; ensure you cancel the previous Task at the start of
restoreFocus and clear the stored Task when it completes.
- Around line 33-35: The nonisolated async function
decodeLargeWorkspacePayload(_:) performs potentially heavy UTF-8 decoding on the
actor's context and can run on the main thread when called from restoreFocus;
annotate decodeLargeWorkspacePayload(_ data: Data) with `@concurrent` to force
background execution under SE-0461, keeping the current String(decoding:as:)
behavior (or add explicit validation/logging elsewhere if you need to handle
malformed bytes differently) so the UTF-8 decode is off the main thread and UI
won't block.
---
Nitpick comments:
In `@Sources/ReviewBotVendorArchitectureEval.swift`:
- Around line 5-7: Replace the ObservableObject/@Published pattern in
ReviewBotVendorFocusCoordinator with the Observation macro: annotate the type
with `@Observable` instead of inheriting ObservableObject and remove `@Published`
from activeWorkspaceID and pendingFocusSurfaceID; update any view call sites
that used `@StateObject` var coordinator to use `@State` (or the appropriate
observation-backed property wrapper) so the coordinator is created and observed
via the Observation framework.
🪄 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: CHILL
Plan: Pro
Run ID: dff87ca7-2292-4b58-8070-c3c25ad81b6b
📒 Files selected for processing (1)
Sources/ReviewBotVendorArchitectureEval.swift
| if !didScheduleRepair { | ||
| didScheduleRepair = true | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { | ||
| self.pendingFocusSurfaceID = Self.lastKnownSurfaceID | ||
| } | ||
| } | ||
|
|
||
| Task { | ||
| let decoded = await decodeLargeWorkspacePayload(payload) | ||
| if decoded.contains(surfaceID.uuidString) { | ||
| try? await Task.sleep(nanoseconds: 150_000_000) | ||
| self.pendingFocusSurfaceID = surfaceID | ||
| } | ||
| } |
There was a problem hiding this comment.
Split focus ownership between DispatchQueue.main.asyncAfter and an untracked Task introduces a correctness race and a logic bug.
Three distinct problems here:
-
Untracked
Task(Lines 24–30): The returnedTaskhandle is discarded. Every call torestoreFocusspawns a new concurrent task that races with all previous ones to overwritependingFocusSurfaceID. There is no cancellation on re-entry. The PR description explicitly calls out "untracked Task with Task.sleep" as an expected finding. -
didScheduleRepairis never reset (Lines 17–22): After the first invocation, the 0.3 sDispatchQueue.main.asyncAfterrepair is never rescheduled. Subsequent calls torestoreFocussilently skip the fallback, which is almost certainly a bug. -
Competing writes with no ownership (Lines 19–28): The
DispatchQueue.main.asyncAfterfires at ~300 ms and theTask.sleepat ~150 ms—both write to the samependingFocusSurfaceIDwith no coordination. This is the "split focus ownership" architectural symptom the PR description highlights.
🐛 Proposed fix — consolidate into a single, cancellable Task
- private var didScheduleRepair = false
+ private var focusTask: Task<Void, Never>?
func restoreFocus(workspaceID: UUID, surfaceID: UUID, payload: Data) {
activeWorkspaceID = workspaceID
pendingFocusSurfaceID = surfaceID
- Self.lastKnownSurfaceID = surfaceID
-
- if !didScheduleRepair {
- didScheduleRepair = true
- DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) {
- self.pendingFocusSurfaceID = Self.lastKnownSurfaceID
- }
- }
-
- Task {
+ focusTask?.cancel()
+ focusTask = Task {
let decoded = await decodeLargeWorkspacePayload(payload)
- if decoded.contains(surfaceID.uuidString) {
- try? await Task.sleep(nanoseconds: 150_000_000)
- self.pendingFocusSurfaceID = surfaceID
+ guard !Task.isCancelled else { return }
+ if decoded.contains(surfaceID.uuidString) {
+ try? await Task.sleep(nanoseconds: 150_000_000)
+ guard !Task.isCancelled else { return }
+ self.pendingFocusSurfaceID = surfaceID
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ReviewBotVendorArchitectureEval.swift` around lines 17 - 30, The
current code spawns an untracked Task and uses DispatchQueue.main.asyncAfter
while never resetting didScheduleRepair, causing racing writes to
pendingFocusSurfaceID; replace both paths with a single cancellable Task stored
on the instance (e.g., a property like pendingFocusRepairTask) that you cancel
on re-entry of restoreFocus, reset or remove the didScheduleRepair flag logic,
and perform a single cooperative sequence inside that Task which awaits
decodeLargeWorkspacePayload(payload) (or sleeps for the fallback delay) and then
sets pendingFocusSurfaceID (using Self.lastKnownSurfaceID as the fallback) so
only this Task owns writes to pendingFocusSurfaceID; ensure you cancel the
previous Task at the start of restoreFocus and clear the stored Task when it
completes.
| nonisolated func decodeLargeWorkspacePayload(_ data: Data) async -> String { | ||
| String(decoding: data, as: UTF8.self) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Swift 6.2 support the @Concurrent attribute on nonisolated async functions (SE-0461)?
💡 Result:
Yes, Swift 6.2 supports the @concurrent attribute on nonisolated async functions as part of SE-0461.
Citations:
- 1: https://bleepingswift.com/blog/swift-6-2-concurrent-attribute
- 2: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0461-async-function-isolation.md
- 3: https://swiftcrafted.dev/article/swift-6-2-approachable-concurrency-complete-guide
- 4: https://www.donnywals.com/exploring-concurrency-changes-in-swift-6-2/
- 5: https://swift.org/blog/swift-6.2-released/
🏁 Script executed:
find . -name "ReviewBotVendorArchitectureEval.swift" -type fRepository: manaflow-ai/cmux
Length of output: 108
🌐 Web query:
SE-0461 AsyncCallerExecution async function isolation semantics
💡 Result:
SE-0461 (“Run nonisolated async functions on the caller’s actor by default”) is about async execution / actor isolation semantics for “nonisolated async” functions. In Swift 6.2, the proposal is implemented, and it introduces a new default plus an explicit opt-in to keep the previous behavior. 1) The key semantic change (default) - A nonisolated async function now runs on the caller’s actor by default. - Concretely, the proposal changes the execution semantics of “nonisolated async” so it becomes consistent with the sync case: nonisolated functions inherit the caller’s actor/executor rather than switching off it by default. - This behavior is described as: nonisolated async functions run on the caller’s actor by default, and the change is gated behind the upcoming feature flag NonisolatedNonsendingByDefault. 2) The explicit way to preserve “switch off the actor” behavior - The proposal provides an explicit annotation to keep the old behavior (switch off the actor and run concurrently with other tasks on the actor): @concurrent (spelled in the proposal as an attribute representing “always hop off the actor”). - When the feature flag is enabled, the default changes to nonisolated(nonsending); @concurrent is the explicit spelling that forces the old “concurrent/off-actor” behavior. 3) The “nonsending” aspect (what crosses isolation boundaries) - The proposal introduces nonisolated(nonsending) as the default target behavior under NonisolatedNonsendingByDefault. - Using nonisolated(nonsending) means that, by default, the argument and result values are not sent over an isolation boundary when the function is called. 4) Why Swift needs more than “just run on the actor” - The proposal explains that this behavior is implemented by implicitly passing an optional actor parameter to the async function; execution uses that actor’s executor. - This allows dynamic inheritance of “which actor to run on” depending on the caller. 5) Relationship to SE-0338 (background) - SE-0338 previously clarified/implemented that non-actor-isolated async functions don’t run on any actor; instead they switch to a generic (non-actor) executor. SE-0461 adjusts the semantics for the specific nonisolated async case by changing the default again. Feature-flag names you may see mentioned - NonisolatedNonsendingByDefault: gates the default behavior change for nonisolated async. Practical takeaway - If you have a nonisolated async function today and you relied on it hopping off the caller’s actor (old default under SE-0338), you should explicitly annotate the function with @concurrent to preserve the old behavior. - Otherwise, with the new default enabled, the function will execute on the caller’s actor, which also affects sendability / what needs to cross an isolation boundary.
Citations:
- 1: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0461-async-function-isolation.md
- 2: https://mehmetbaykar.com/posts/se-0461-async-actor-isolation-swift-6-2/
- 3: https://github.com/apple/swift-evolution/blob/main/proposals/0338-clarify-execution-non-actor-async.md
- 4: Sema: Implement adoption mode for
AsyncCallerExecutionswiftlang/swift#80008 - 5: [Frontend] Rename
NonIsolatedAsyncInheritsIsolationFromContextfeature and add a feature for@executionswiftlang/swift#79508
🏁 Script executed:
sed -n '30,45p' ./Sources/ReviewBotVendorArchitectureEval.swiftRepository: manaflow-ai/cmux
Length of output: 514
🏁 Script executed:
grep -n "restoreFocus\|decodeLargeWorkspacePayload" ./Sources/ReviewBotVendorArchitectureEval.swiftRepository: manaflow-ai/cmux
Length of output: 339
🏁 Script executed:
sed -n '12,28p' ./Sources/ReviewBotVendorArchitectureEval.swiftRepository: manaflow-ai/cmux
Length of output: 752
🌐 Web query:
Swift String(bytes:encoding:) vs String(decoding:as:) difference failable
💡 Result:
String(bytes:encoding:) is a failable initializer (returns String?) that attempts to interpret a sequence of UInt8 bytes using a String.Encoding (like .utf8), returning nil if the bytes cannot be validly decoded in that encoding. It performs lossy decoding for ill-formed sequences in some cases but fails for certain invalid inputs. String(decoding:as:) is a non-failable initializer (returns String) that decodes a sequence of Unicode code units (e.g., [UInt8] for UTF8.self) using a specific Unicode codec. It always succeeds, replacing any ill-formed sequences with the Unicode replacement character U+FFFD. The key difference regarding failable behavior: String(bytes:encoding:) can fail (failable), while String(decoding:as:) cannot (non-failable, lossy). String(bytes:encoding:) uses legacy Foundation String.Encoding (which maps to Unicode codecs but with validation), while String(decoding:as:) uses modern native Swift Unicode codecs directly. Prefer String(decoding:as:) for new code as it's the recommended native API.
Citations:
- 1: https://developer.apple.com/documentation/swift/string/init(bytes:encoding:)
- 2: https://sosumi.ai/documentation/swift/string/init(bytes:encoding:)
- 3: https://developer.apple.com/documentation/swift/string
- 4: String encoding/decoding is a mess swiftlang/swift#69253
- 5: [SR-6261] String(decoding:as:) is about 100% slower than String(data:encoding:) for UTF-8 Data swiftlang/swift#48811
- 6: https://www.hackingwithswift.com/example-code/language/how-to-convert-data-to-a-string
🏁 Script executed:
sed -n '1,50p' ./Sources/ReviewBotVendorArchitectureEval.swift | grep -E "@MainActor|class|struct"Repository: manaflow-ai/cmux
Length of output: 187
🏁 Script executed:
cat ./Sources/ReviewBotVendorArchitectureEval.swift | head -15Repository: manaflow-ai/cmux
Length of output: 555
🏁 Script executed:
find . -name "*.md" -type f | xargs grep -l "decodeLargeWorkspacePayload\|concurrent.*decoding" 2>/dev/null | head -5Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
git log --oneline --all -20 2>/dev/null | head -20Repository: manaflow-ai/cmux
Length of output: 1251
🏁 Script executed:
git show 8068137 --stat 2>/dev/null | head -30Repository: manaflow-ai/cmux
Length of output: 1769
🏁 Script executed:
git show 8068137 2>/dev/null | grep -A 50 "^Author\|^Date\|missing.*concurrent\|heavy.*decoding" | head -60Repository: manaflow-ai/cmux
Length of output: 2216
Add @concurrent to decodeLargeWorkspacePayload to force off-main execution.
Under SE-0461, the nonisolated async function inherits the caller's MainActor context by default when called from restoreFocus inside a Task. For a potentially large payload, UTF-8 decoding on the main thread blocks the UI. Annotate with @concurrent to explicitly opt back into background execution.
Regarding the string decoding approach: while String(decoding:as:) is lossy (silently replacing invalid bytes with U+FFFD), it is Swift's recommended modern API for new code. If handling malformed payloads explicitly is required, consider documenting the lossy behavior or using a validation/logging layer rather than switching to the legacy String(bytes:encoding:) API.
🔧 Suggested change
- nonisolated func decodeLargeWorkspacePayload(_ data: Data) async -> String {
+ `@concurrent`
+ nonisolated func decodeLargeWorkspacePayload(_ data: Data) async -> String {🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 34-34: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ReviewBotVendorArchitectureEval.swift` around lines 33 - 35, The
nonisolated async function decodeLargeWorkspacePayload(_:) performs potentially
heavy UTF-8 decoding on the actor's context and can run on the main thread when
called from restoreFocus; annotate decodeLargeWorkspacePayload(_ data: Data)
with `@concurrent` to force background execution under SE-0461, keeping the
current String(decoding:as:) behavior (or add explicit validation/logging
elsewhere if you need to handle malformed bytes differently) so the UTF-8 decode
is off the main thread and UI won't block.
| var body: some View { | ||
| Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing") | ||
| .onAppear { | ||
| coordinator.restoreFocus( | ||
| workspaceID: workspaceID, | ||
| surfaceID: surfaceID, | ||
| payload: payload | ||
| ) | ||
| } |
There was a problem hiding this comment.
restoreFocus is called from .onAppear, violating the guideline against state mutation in view-body computations.
restoreFocus writes @Published (activeWorkspaceID, pendingFocusSurfaceID), schedules a DispatchQueue.main.asyncAfter, and spawns a Task—all triggered from .onAppear inside body. The guideline is explicit:
"Do not add state mutation inside view-body computations. Functions called from
bodymust not write@Publishedstate, schedule state-changing tasks, or queue store writes."
The coordinator should receive its inputs at construction time (or via a dedicated task(id:) modifier keyed to identity changes) rather than side-effecting from the view body.
♻️ Suggested restructure
struct ReviewBotVendorArchitectureEvalPanel: View {
- `@StateObject` private var coordinator = ReviewBotVendorFocusCoordinator()
let workspaceID: UUID
let surfaceID: UUID
let payload: Data
+ `@State` private var coordinator: ReviewBotVendorFocusCoordinator
+ init(workspaceID: UUID, surfaceID: UUID, payload: Data) {
+ self.workspaceID = workspaceID
+ self.surfaceID = surfaceID
+ self.payload = payload
+ _coordinator = State(wrappedValue: ReviewBotVendorFocusCoordinator())
+ }
var body: some View {
Text(coordinator.pendingFocusSurfaceID?.uuidString ?? ...)
- .onAppear {
- coordinator.restoreFocus(
- workspaceID: workspaceID,
- surfaceID: surfaceID,
- payload: payload
- )
- }
+ .task(id: surfaceID) {
+ await coordinator.restoreFocus(
+ workspaceID: workspaceID,
+ surfaceID: surfaceID,
+ payload: payload
+ )
+ }
}
}As per coding guidelines: "Do not add state mutation inside view-body computations. Functions called from body must not write @Published state, schedule state-changing tasks, or queue store writes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ReviewBotVendorArchitectureEval.swift` around lines 44 - 52, The view
is calling coordinator.restoreFocus from .onAppear which mutates `@Published`
state and schedules async work; move this side-effect out of the view-body by
either passing the needed inputs into the coordinator at construction or
invoking restoreFocus from a SwiftUI lifecycle modifier that is safe for state
mutation (e.g. use .task(id: workspaceID, surfaceID, payload) or call
restoreFocus from the parent when creating the coordinator), so locate the call
to restoreFocus (and the Text view using coordinator.pendingFocusSurfaceID) and
replace the .onAppear invocation with a construction-time call or a .task(id:)
invocation keyed to workspaceID/surfaceID/payload to prevent state mutation
during body computation.
| let payload: Data | ||
|
|
||
| var body: some View { | ||
| Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing") |
There was a problem hiding this comment.
Unlocalized fallback string "missing" violates the localization guideline.
"missing" is a user-facing string rendered directly in a Text view. The guideline requires every string shown in the UI to use String(localized:defaultValue:) with a key in Resources/Localizable.xcstrings. The PR description explicitly lists "unlocalized Text fallback" as an expected finding.
🌍 Proposed fix
- Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing")
+ Text(coordinator.pendingFocusSurfaceID?.uuidString ?? String(localized: "focusCoordinator.surface.missing", defaultValue: "missing"))Add the key to Resources/Localizable.xcstrings with English and Japanese translations.
As per coding guidelines: "All user-facing strings must be localized. Use String(localized: "key.name", defaultValue: "English text") for every string shown in UI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ReviewBotVendorArchitectureEval.swift` at line 45, The Text view
currently uses an unlocalized fallback literal ("missing") in the expression
Text(coordinator.pendingFocusSurfaceID?.uuidString ?? "missing"); replace this
literal with a localized string using String(localized:
"reviewbot.pendingFocus.missing", defaultValue: "missing") and add the
"reviewbot.pendingFocus.missing" key with English and Japanese entries to
Resources/Localizable.xcstrings so the UI string is localized.
|
Closing temporary vendor lint eval PR. Results were collected and used for #3496. |
This branch was successfully deployed
1 participant
Temporary eval PR. Do not merge.
This intentionally adds a focused Swift fixture after #3493 merged, so Greptile and CodeRabbit use the base-branch vendor rules.
Expected findings: architectural symptom patching with split focus ownership, delayed DispatchQueue repair, untracked Task with Task.sleep, legacy ObservableObject/@published, missing @Concurrent on heavy nonisolated async decoding, and unlocalized Text fallback.
@coderabbitai review
@greptile-apps review
Summary by cubic
Adds a temporary Swift fixture to evaluate vendor architecture and concurrency lint rules using the base-branch config. It simulates a focus-restore flow in SwiftUI to surface expected rule findings.
Written for commit 8068137. Summary will update on new commits.
Summary by CodeRabbit
Release Notes