Interpreter primitives: leaf-tier SwiftUI coverage (composability) - #5275
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds a shared recursion budget and runs deep parsing/evaluation on a large-stack worker, extends expression evaluation (builtins, array/string methods, switch and optional-binding conditions, multi-statement closures), expands view/ modifier handling (new node kinds, gradients, progress/gauge, child-bearing modifiers), and includes a large test corpus and documentation for a future ChangesRecursion-Safe SwiftUI Sidebar Interpreter Core
Expression Evaluation and Value Processing
View Interpreter and Rendering Pipeline
Rendering Output and Style Resolution
Testing, Coverage, and Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift">
<violation number="1" location="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift:50">
P2: `.section` is rendered as `VStack` instead of SwiftUI `Section`, so section nodes lose list section semantics and render as a single row block.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| VStack(alignment: .leading, spacing: 4) { | ||
| if let header = node.text, !header.isEmpty { | ||
| Text(header) | ||
| .font(.caption) | ||
| .fontWeight(.semibold) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| children | ||
| } | ||
| .frame(maxWidth: .infinity, alignment: .leading) |
There was a problem hiding this comment.
P2: .section is rendered as VStack instead of SwiftUI Section, so section nodes lose list section semantics and render as a single row block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift, line 50:
<comment>`.section` is rendered as `VStack` instead of SwiftUI `Section`, so section nodes lose list section semantics and render as a single row block.</comment>
<file context>
@@ -34,6 +34,33 @@ struct RenderNodeView: View {
+ .listStyle(.plain)
+ .scrollContentBackground(.hidden)
+ case .section:
+ VStack(alignment: .leading, spacing: 4) {
+ if let header = node.text, !header.isEmpty {
+ Text(header)
</file context>
| VStack(alignment: .leading, spacing: 4) { | |
| if let header = node.text, !header.isEmpty { | |
| Text(header) | |
| .font(.caption) | |
| .fontWeight(.semibold) | |
| .foregroundStyle(.secondary) | |
| } | |
| children | |
| } | |
| .frame(maxWidth: .infinity, alignment: .leading) | |
| Section { | |
| children | |
| } header: { | |
| if let header = node.text, !header.isEmpty { | |
| Text(header) | |
| .font(.caption) | |
| .fontWeight(.semibold) | |
| .foregroundStyle(.secondary) | |
| } | |
| } |
Greptile SummaryThis PR implements the leaf-tier SwiftUI coverage matrix: 20+ new node kinds (List, Grid variants, gradients, ProgressView, Gauge, Menu, Label, LazyVStack/LazyHStack, Group, and shape variants), ~30 new modifier cases (full text/typography, decoration, SF Symbol, interaction),
Confidence Score: 4/5Safe to merge once the open thread items from the previous review cycle are resolved; the new code is well-structured and all 56 interpreter tests pass. The main gaps carried from the previous cycle —
Important Files Changed
Sequence DiagramsequenceDiagram
participant Host as CustomSidebarView
participant Interp as SwiftViewInterpreter
participant Worker as LargeStack Thread
participant Budget as RecursionBudget
participant EE as ExpressionEvaluator
participant Render as RenderNodeView
Host->>Interp: parse(source)
Interp->>Worker: "onLargeStack { Parser.parse + foldAll }"
Worker-->>Interp: ParsedProgram (done.signal)
Interp-->>Host: ParsedProgram
Host->>Interp: evaluate(program, state:)
Interp->>Worker: "onLargeStack { evalView }"
Worker->>Budget: enter() / exceeded?
Worker->>EE: eval(expr, env)
EE->>Budget: enter() / exceeded?
EE-->>Worker: SwiftValue
Worker->>Budget: leave()
Worker-->>Interp: RenderNode tree (done.signal)
Interp-->>Host: RenderNode?
Host->>Render: RenderNodeView(node:)
Render->>Render: renderKind (List/Grid/Gradient/…)
Render->>Render: applyModifiers (overlay/background/mask/…)
Render-->>Host: SwiftUI View
Reviews (10): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| private func scrollViewIsHorizontal(_ call: FunctionCallExprSyntax, _ env: Environment) -> Bool { | ||
| guard let axes = call.arguments.first(where: { $0.label == nil })?.expression else { return false } | ||
| return axes.trimmedDescription.contains("horizontal") | ||
| } |
There was a problem hiding this comment.
Text-contains check misclassifies bidirectional scroll views
trimmedDescription.contains("horizontal") returns true for ScrollView([.vertical, .horizontal]) { ... } because the string [.vertical, .horizontal] contains the substring "horizontal". The node is then lowered to .hscroll, which wraps children in a plain HStack — silently dropping the vertical-scroll axis. The sidebar already passthrough-strips vertical ScrollView to avoid double-scroll, but a bidirectional declaration is neither horizontal-only nor a plain vertical passthrough, so its intent is lost entirely.
| case .list: | ||
| // Plain, chrome-light list so it sits naturally in the sidebar | ||
| // rather than imposing inset grouped-table styling. | ||
| List { children } | ||
| .listStyle(.plain) | ||
| .scrollContentBackground(.hidden) |
There was a problem hiding this comment.
List is itself a scroll view — same double-scroll risk that motivated the ScrollView passthrough
SwiftUI List embeds its own ScrollView internally. The PR's rationale for keeping vertical ScrollView as a vstack passthrough was that "the sidebar already scrolls vertically" and a second scroll view would double-scroll. A .list node lands the same problem: when the sidebar host provides a fixed-height slot the inner List can work, but when it's placed in a flexible VStack or Group without a bounded height it will either collapse to zero or fight with the outer scroll geometry. Unlike ScrollView, List cannot easily be made passthrough because its row-reuse and separator logic are intrinsic to the List itself. At minimum the node needs a .frame(maxHeight:) cap or a design note explaining how the host always provides a bounded height.
| case .section: | ||
| VStack(alignment: .leading, spacing: 4) { | ||
| if let header = node.text, !header.isEmpty { | ||
| Text(header) | ||
| .font(.caption) | ||
| .fontWeight(.semibold) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| children | ||
| } | ||
| .frame(maxWidth: .infinity, alignment: .leading) |
There was a problem hiding this comment.
Section renders as a plain VStack even when nested inside a List, losing list-section semantics
When the node tree contains list → section → …, the .section case renders as a custom VStack + Text header rather than a SwiftUI Section inside the List. This loses sticky headers, the separator line between sections, and any listSectionHeader styling that the platform would apply. A Section child of a .list node is the most common use of Section; using it only outside a list is unusual. Consider delegating to a proper SwiftUI Section at render time when the parent context is a List.
| case .section: | |
| VStack(alignment: .leading, spacing: 4) { | |
| if let header = node.text, !header.isEmpty { | |
| Text(header) | |
| .font(.caption) | |
| .fontWeight(.semibold) | |
| .foregroundStyle(.secondary) | |
| } | |
| children | |
| } | |
| .frame(maxWidth: .infinity, alignment: .leading) | |
| case .section: | |
| if let header = node.text, !header.isEmpty { | |
| Section(header: Text(header) | |
| .font(.caption) | |
| .fontWeight(.semibold) | |
| .foregroundStyle(.secondary) | |
| ) { | |
| children | |
| } | |
| } else { | |
| Section { children } | |
| } |
| case "List": | ||
| return RenderNode(kind: .list, children: call.trailingClosure.map { evalItems($0.statements, env) } ?? []) | ||
| case "Section": | ||
| // `Section("Header") { ... }` / `Section { ... }`: the leading | ||
| // string literal (if any) becomes the header above the content. | ||
| return RenderNode( | ||
| kind: .section, | ||
| text: stringArgument(call.arguments, env), | ||
| children: call.trailingClosure.map { evalItems($0.statements, env) } ?? [] | ||
| ) |
There was a problem hiding this comment.
Section header extraction only handles positional string literals
stringArgument grabs args.first, which works for Section("Repos") { ... } but silently returns nil for the common Section(header: Text("Repos")) { ... } form (labeled header: argument with a Text view). In that form the header is a ViewBuilder closure, not a string, so the header text is lost entirely. If the authored SwiftUI uses the header: label, the section will render without a title and no diagnostic is emitted.
There was a problem hiding this comment.
5 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swift">
<violation number="1" location="Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swift:91">
P2: `min`/`max` silently ignore invalid arguments because `compactMap` drops them, producing results from partial input instead of returning `nil`.</violation>
</file>
<file name="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift">
<violation number="1" location="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift:50">
P2: `.section` is rendered as `VStack` instead of SwiftUI `Section`, so section nodes lose list section semantics and render as a single row block.</violation>
<violation number="2" location="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift:207">
P2: `safeAreaInset(edge:)` incorrectly maps all non-top edges to `.bottom`, so `leading`/`trailing` insets render on the wrong side.</violation>
</file>
<file name="Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift">
<violation number="1" location="Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift:190">
P2: `ProgressView` text extraction is using the first argument even when it is `value:`, causing numeric progress values to be rendered as labels.</violation>
<violation number="2" location="Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift:196">
P2: `Gauge` title fallback reads labeled `value:` as text, so value-only gauges get incorrect numeric labels.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let ref = call.calledExpression.as(DeclReferenceExprSyntax.self), | ||
| ["min", "max", "abs"].contains(ref.baseName.text), | ||
| env.lookupFunction(ref.baseName.text) == nil { | ||
| let nums = call.arguments.compactMap { numericValue(eval($0.expression, env)) } |
There was a problem hiding this comment.
P2: min/max silently ignore invalid arguments because compactMap drops them, producing results from partial input instead of returning nil.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swift, line 91:
<comment>`min`/`max` silently ignore invalid arguments because `compactMap` drops them, producing results from partial input instead of returning `nil`.</comment>
<file context>
@@ -57,6 +57,45 @@ struct ExpressionEvaluator {
+ let ref = call.calledExpression.as(DeclReferenceExprSyntax.self),
+ ["min", "max", "abs"].contains(ref.baseName.text),
+ env.lookupFunction(ref.baseName.text) == nil {
+ let nums = call.arguments.compactMap { numericValue(eval($0.expression, env)) }
+ switch ref.baseName.text {
+ case "min" where nums.count >= 2: return numberResult(nums.min()!, intIf: allInt(call, env))
</file context>
| case "safeAreaInset": | ||
| if !modifier.children.isEmpty { | ||
| let edge = clean(modifier.value("edge")) | ||
| if edge == "top" { |
There was a problem hiding this comment.
P2: safeAreaInset(edge:) incorrectly maps all non-top edges to .bottom, so leading/trailing insets render on the wrong side.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift, line 207:
<comment>`safeAreaInset(edge:)` incorrectly maps all non-top edges to `.bottom`, so `leading`/`trailing` insets render on the wrong side.</comment>
<file context>
@@ -123,17 +158,58 @@ struct RenderNodeView: View {
+ case "safeAreaInset":
+ if !modifier.children.isEmpty {
+ let edge = clean(modifier.value("edge"))
+ if edge == "top" {
+ return AnyView(view.safeAreaInset(edge: .top) { modifierChildren(modifier) })
+ }
</file context>
| case "Gauge": | ||
| return RenderNode( | ||
| kind: .gauge, | ||
| text: labeledStringArgument("label", call.arguments, env) ?? stringArgument(call.arguments, env), |
There was a problem hiding this comment.
P2: Gauge title fallback reads labeled value: as text, so value-only gauges get incorrect numeric labels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift, line 196:
<comment>`Gauge` title fallback reads labeled `value:` as text, so value-only gauges get incorrect numeric labels.</comment>
<file context>
@@ -147,6 +171,37 @@ public struct SwiftViewInterpreter: Sendable {
+ case "Gauge":
+ return RenderNode(
+ kind: .gauge,
+ text: labeledStringArgument("label", call.arguments, env) ?? stringArgument(call.arguments, env),
+ value: doubleArgument(named: "value", call.arguments, env)
+ )
</file context>
| text: labeledStringArgument("label", call.arguments, env) ?? stringArgument(call.arguments, env), | |
| text: labeledStringArgument("label", call.arguments, env) ?? call.arguments.first(where: { $0.label == nil }).flatMap { exprString($0.expression, env) }, |
| case "ProgressView": | ||
| return RenderNode( | ||
| kind: .progressView, | ||
| text: stringArgument(call.arguments, env), |
There was a problem hiding this comment.
P2: ProgressView text extraction is using the first argument even when it is value:, causing numeric progress values to be rendered as labels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift, line 190:
<comment>`ProgressView` text extraction is using the first argument even when it is `value:`, causing numeric progress values to be rendered as labels.</comment>
<file context>
@@ -147,6 +171,37 @@ public struct SwiftViewInterpreter: Sendable {
+ case "ProgressView":
+ return RenderNode(
+ kind: .progressView,
+ text: stringArgument(call.arguments, env),
+ value: progressValue(call, env)
+ )
</file context>
| text: stringArgument(call.arguments, env), | |
| text: call.arguments.first(where: { $0.label == nil }).flatMap { exprString($0.expression, env) }, |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
| public func evaluate(_ source: String, state: [String: SwiftValue] = [:]) -> RenderNode? { | ||
| // RenderNode is Sendable; the box is written once on the worker and read | ||
| // only after `join`, so the unchecked Sendable is safe. | ||
| final class ResultBox: @unchecked Sendable { var node: RenderNode? } | ||
| let box = ResultBox() | ||
| let done = DispatchSemaphore(value: 0) // one-shot thread-join signal, not a state lock. | ||
| let worker = Thread { | ||
| box.node = self.interpret(source, state: state) | ||
| done.signal() | ||
| } | ||
| worker.stackSize = 16 * 1024 * 1024 | ||
| worker.start() | ||
| done.wait() | ||
| return box.node |
There was a problem hiding this comment.
DispatchSemaphore.wait() blocks the main thread on every sidebar render
evaluate() is called from CustomSidebarView.content — a @ViewBuilder that runs on the main thread whenever dataContext changes. The done.wait() on line 57 parks the main thread until the worker finishes parsing + interpreting, which can be tens to hundreds of milliseconds on a complex corpus sidebar. That directly blocks UI rendering, input handling, and the run loop for the entire duration.
The cmux-swift-blocking-runtime rule prohibits DispatchSemaphore.wait() in production Swift; the // one-shot thread-join signal comment correctly describes the intent but does not change what the call does on the caller thread.
The large-stack requirement is real and correct, but the interpretation should be moved out of the view body: run it in the model/actor on an explicit Task that kicks off a new Thread, resume via withCheckedContinuation, and store the resulting RenderNode in model state. The view then just reads model state rather than blocking on it.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
…EmptyView + horizontal ScrollView
Leaf-tier container coverage from docs/swiftui-interpreter-surface.md:
- New RenderNode kinds: lazyVStack, lazyHStack, group, list, section, hscroll.
- Interpreter constructors for LazyVStack/LazyHStack, Group, EmptyView (-> empty
group), List, Section("Header") { } (leading string literal becomes header),
and ScrollView(.horizontal) -> hscroll (vertical stays passthrough so it does
not double-scroll inside the already-scrolling sidebar).
- RenderNodeView renders each: LazyVStack/LazyHStack, transparent Group, plain
chrome-light List (.plain + hidden scroll background), Section with a styled
caption header above its content, and a horizontal ScrollView+HStack.
- Harness allow-list + 3 behavior tests (37 interpreter tests pass; UI package
builds).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…slicing + conversions
- ForEach over `Array(xs.enumerated())` with a 2-arg closure `{ index, item in }`
now destructures the pair (offset/element). `.enumerated()` yields pairs
addressable as `.offset`/`.element` or `$0`/`$1`.
- `.indices` resolves as a property (bare, no parens) on arrays, so
`ForEach(xs.indices) { i in ... }` works; also `.first`/`.last` properties.
- Array `.dropFirst(_:)`, `.dropLast(_:)`, `.suffix(_:)` methods.
- `Array(seq)` identity passthrough and `Int()/Double()/String()` conversions in
value position.
- Allow-list + 3 behavior tests (40 interpreter tests pass).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- New view-level text modifiers in the bridge: .italic, .underline, .monospaced, .monospacedDigit, .fontDesign, .multilineTextAlignment, .textCase, .truncationMode (resolvers in RenderStyle). All apply to the erased view, so they affect contained text. - Label(title, systemImage:) -> new .label kind rendered as a native Label. - Allow-list (Label/Array/Int/Double/String + the new members) + 2 tests (42 interpreter tests pass; UI builds). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…orm modifiers - New shape kinds: Ellipse, UnevenRoundedRectangle (uniform-radius approx). - Widened dslColor palette: tertiary/quaternary + mint/teal/cyan/indigo/brown. - New bridge modifiers: shadow(color:radius:x:y:), border(_,width:), blur(radius:), offset(x:y:), scaleEffect, rotationEffect(.degrees/.radians), zIndex, brightness/contrast/saturation/grayscale, clipShape(<shape>), clipped, fixedSize, layoutPriority. Angle token parser for rotationEffect. - Allow-list + 2 tests (44 interpreter tests pass; UI builds). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…round/.mask/.safeAreaInset)
The composability keystone: RenderModifier now carries an optional child
RenderNode subtree captured from a modifier's trailing closure. The interpreter
lowers `.overlay { ... }`, `.background { ... }`, `.mask { ... }`, and
`.safeAreaInset(edge:) { ... }` content through evalItems, and the bridge renders
it (alignment-aware for overlay/background; ZStack for multiple children). This
lets interpreted views nest arbitrary content inside modifiers, not just colors.
Background/overlay still accept a plain color token when there is no closure.
Allow-list + a test (45 interpreter tests pass; UI builds).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Bridge modifiers: .imageScale, .symbolRenderingMode (hierarchical/multicolor/ palette/monochrome), .symbolVariant (fill/circle/square/slash) — the symbol controls SF-symbol sidebars actually use. - Numeric builtins min(_,_)/max(_,_)/abs(_) in value position (int-preserving). - Allow-list + 2 tests (47 interpreter tests pass; UI builds). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tFits - New kinds grid/gridRow/lazyVGrid/lazyHGrid/viewThatFits with interpreter constructors and native bridge rendering. Grid uses leading alignment; LazyV/HGrid use adaptive columns/rows (sidebar-appropriate approximation of the GridItem spec); ViewThatFits lets SwiftUI pick the first fitting child. - Allow-list + a test (48 interpreter tests pass; UI builds). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/disabled
- New kinds progressView (determinate via normalized value:/total: or
indeterminate), gauge, menu (Menu(title) { items }). RenderNode gains a `value`
field for determinate controls.
- contextMenu joins the child-bearing modifiers (its closure items render as the
menu); .help (tooltip) and .disabled bridge modifiers.
- Allow-list + 2 tests (49 interpreter tests pass; UI builds).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ns, aspectRatio Generic view-level bridge modifiers (no IR change; the interpreter already captures them): .redacted(reason:)/.unredacted, .accessibilityLabel/Hint/Value/ Hidden, .scrollIndicators, .scrollContentBackground, .aspectRatio(contentMode:), .scaledToFit/.scaledToFill. Allow-list updated; UI builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… corpus Deeply-composed sidebars (and pathological nesting) overflowed the caller stack during parse/interpret and crashed the process (SIGBUS). Fixes: - evaluate() now runs parse + interpret on a dedicated 16MB-stack Thread, so realistic deep nesting renders instead of overflowing the small caller stack. - A per-Environment RecursionBudget (shared down the scope chain) backstops genuinely unbounded interpreter recursion (mutually-recursive view helpers), guarding evalView/eval/evalItems/callValueFunction. - Add 8 multi-agent-authored stress sidebars to the corpus (deeply nested overlays/backgrounds, grids, menus, progress, data-driven). All 22 corpus sidebars now render; 50 interpreter tests pass, incl a 600-deep nesting test. Surfaced next gap: shape `.stroke` (13 uses) / `.trim` / `.strokeBorder`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… switch
- evalIf now evaluates condition lists via conditionsPass, binding
`if let name = expr` (and shorthand `if let name`) optionals into the
then-branch scope (previously only boolean conditions were honored, so
`if let` never rendered).
- View-position `switch subject { case "x": ... default: ... }` selects the
first matching case; matches string/int literal patterns and bare `.member`
enum-style patterns against the subject. The type-system tier begins.
- 2 tests (55 interpreter tests pass).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ue funcs
evalBlockValue/evalIfValue now mirror the view-side language support: a value
helper can use `switch x { case ...: return ... }` and `if let y = opt { return y }`
(conditionsPass binds optionals into the branch scope). Lets authors write
natural value helpers (e.g. a status->color switch). 56 interpreter tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second multi-agent stress round, now leaning on the language tier (value-func
`switch` mapping status->color/symbol, `if let pr = w.pr`), gradients in
`.background { }`, `.trim` arcs, AngularGradient. All 28 corpus sidebars render
(0 NIL); permanent regression fixtures for the language + gradient tiers. Only
remaining unsupported symbol is `.resizable` (+ undefined persona helper funcs,
which are authoring gaps, not interpreter gaps).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
styledImage applies .resizable() on the concrete Image (an Image method, not available post-AnyView-erasure); .scaledToFit/.aspectRatio from the generic pass apply on top. Closes the last unsupported modifier from the stress corpus — the only remaining gaps are AsyncImage/URL (async/file) and the @State tier. UI builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Confirmed findings from a multi-agent review of tonight's changes:
- CRITICAL: evalClosure/evalClosure2 returned the first expression, skipping
local `let` bindings — `xs.map { x in let d = x*2; d+1 }` (and reduce/sorted
with locals) silently broke. Both now route the body through evalBlockValue,
honoring let/if/switch + trailing expression.
- HIGH: Color(red:green:blue:) channel did `Int(d*255)` which TRAPS on
Double.infinity/NaN — guard `isFinite` + clamp to [0,1] before converting.
- range iterationValues: overflow-safe end (addingReportingOverflow) + a 100k
materialization cap so `0...Int.max` can't overflow or exhaust memory.
- .disabled() now disables only when the arg is explicitly true (unresolved
expr -> enabled, not disabled).
- .aspectRatio applies an explicit ratio only when positive (zero/negative is
invalid in SwiftUI) -> falls back to mode-only.
Regression tests for the closure-let and non-finite-color cases (58 tests pass).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Common value-method long-tail: array `.joined(separator:)`, string `.replacingOccurrences(of:with:)` (methods, via evalMethod), and `.capitalized` (a property — resolved in SwiftValue.member like .indices, alongside .uppercased/.lowercased property forms). 59 interpreter tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Common everyday helper (trim titles/branches): .trimmingCharacters(in: .whitespaces / .whitespacesAndNewlines / .newlines). 60 interpreter tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#5254 merged the parse()->ParsedProgram / evaluate(program:) split into main. Reconcile with this branch's 16MB-stack robustness fix: both parse() and evaluate(program:) now run their recursive work on a shared onLargeStack worker (LargeStackResultBox), and ParsedProgram is Sendable so it can cross the worker boundary (also addresses CodeRabbit's Sendable suggestion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f0b852e to
3982972
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/custom-sidebars.md`:
- Around line 235-240: The "Still missing" list is out of date; update
docs/custom-sidebars.md to reflect current interpreter capabilities by removing
or reclassifying items that are now supported (specifically remove or mark as
implemented: switch, gradients/LinearGradient, and .resizable) and ensure
remaining entries still accurately list unsupported features (e.g., `@State`,
TextField, Toggle, Slider, Picker two-way binding, custom struct/View
definitions, navigation APIs like sheet/popover/NavigationStack,
.keyboardShortcut, AsyncImage/.resizable usage nuances, and workspace data items
such as git branch/dirty/ports/PR/unread/remote/latest agent/prompt messages);
keep references to cmux(...) behavior and any interactive control limitations
and make the wording consistent with shipped tests and surface.
In `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swift`:
- Around line 8-34: Add a clarifying doc comment to RecursionBudget stating that
instances are not thread-safe and are intended to be used only on a single
evaluation/worker thread (e.g., bound to the onLargeStack evaluation session) so
that mutable state (depth) can be shared across an Environment chain without
concurrent access; refer to the class name RecursionBudget and its mutable
properties/methods (depth, enter(), leave(), exceeded, init(limit:)) so
maintainers know the lifecycle and concurrency assumptions.
In `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swift`:
- Around line 68-71: The current logic forms end = upper + 1 for inclusive
ranges which overflows at Int.max; instead compute the distance safely without
doing upper + 1: use upper.subtractingReportingOverflow(lower) to get (diff,
subOverflow), guard !subOverflow, then if inclusive add one to diff with
diff.addingReportingOverflow(1) and guard that adding did not overflow; set
count to the resulting value and then guard count <= 100_000. Replace references
to end/addOverflow with this safe two-step subtract-then-conditional-add flow
using the existing variables (inclusive, upper, lower, count, subOverflow) and
remove the upper.addingReportingOverflow(1) path.
In `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift`:
- Around line 85-101: The DispatchSemaphore-based join in onLargeStack (uses
done and worker) blocks the caller thread which contradicts the project's
guideline to avoid blocking waits; either replace the semaphore join with a
higher-level sync primitive (e.g., run the work on an Operation/BlockOperation
and call operationQueue.waitUntilAllOperationsAreFinished()) to avoid manual
semaphores, or if keeping the current Thread + DispatchSemaphore approach, add a
clear comment and a defensive precondition to prevent calling on the main thread
(e.g., assert(!Thread.isMainThread)) and document the rationale in onLargeStack
so it aligns with the synchronous API and project concurrency policy. Ensure
references to LargeStackResultBox<T>, done, and worker remain consistent when
making the change.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swift`:
- Around line 119-124: The overlayed status glyph is permanently hidden because
Image(systemName: glyph) has .opacity(0.0); update the overlay so the glyph is
visible (e.g., remove the opacity modifier or set it to 1.0) so the computed
glyph (glyph) rendered in the .overlay block actually appears; locate the
overlay containing Image(systemName: glyph) and adjust or remove the
.opacity(...) call accordingly.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swift`:
- Around line 157-221: The ForEach blocks are iterating plain [[String]] but use
id: \.offset with a two-parameter closure ({ i, cell in }) which requires an
enumerated collection; wrap the literal arrays with Array(...enumerated()) so
the data shape matches other corpus files. Update both ForEach occurrences (the
two status tile grids that build VStack using prGlyph(_:), prColor(_:), and
statusLabel(_:)) to use ForEach(Array([ ... ].enumerated()), id: \.offset) so
the closure params (i, cell) and id: \.offset work correctly.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-keyboard-first-workspace-index.swift`:
- Around line 244-263: Replace the view-builder native "for p in pinned" loop
with SwiftUI's ForEach so the views render in the interpreter: locate the
ScrollView/HStack block that iterates over the pinned collection (the closure
creating HStack(spacing:4) for each p) and change it to ForEach(pinned, id:
\.id) { p in ... } (or an appropriate id/keypath) so each item is produced by
the view builder; ensure the inner HStack, Image, Text styling and the
.onTapGesture calling cmux("workspace.select", workspace_id: p.id) remain
unchanged.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swift`:
- Around line 640-643: The test colorChannelHandlesNonFiniteWithoutCrashing
currently evaluates a string with Color(red: 2.0, ...) so it never exercises the
non-finite path; update the eval string passed to interp.evaluate in
SwiftViewInterpreterTests.swift (inside the
colorChannelHandlesNonFiniteWithoutCrashing test) to use a non-finite literal
such as .infinity or .nan for the red channel (e.g. Color(red: .infinity,...))
so the Double->Int conversion path for non-finite values is exercised;
optionally add a second assertion or separate subtest that uses .nan to cover
both cases and keep the existing assertion that the last child text == "ok".
In
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`:
- Around line 114-117: RenderNode.Kind.unevenRoundedRectangle is currently
flattened to a uniform RoundedRectangle because RenderNode only stores a single
cornerRadius; update the IR and lowering so unevenRoundedRectangle carries four
corner radii (topLeading, topTrailing, bottomLeading, bottomTrailing), modify
SwiftViewInterpreter to parse all four radii into RenderNode (falling back to
the existing cornerRadius when per-corner values are absent), and change
RenderNodeView's case for .unevenRoundedRectangle (and the styledShape call) to
produce SwiftUI UnevenRoundedRectangle(...) using those four radii instead of
RoundedRectangle(cornerRadius:).
- Around line 49-59: RenderNodeView currently uses a VStack for case .section
which breaks SwiftUI List semantics; change the .section branch to emit a
SwiftUI Section when it may appear inside a List: replace the VStack with
Section(header: { if let header = node.text, !header.isEmpty {
Text(header).font(.caption).fontWeight(.semibold).foregroundStyle(.secondary) }
}, content: { children }) so the header and children are actual Section parts
(preserving the same Text styling and alignment) and remove the manual .frame
used for the VStack; keep the existing children view and ensure this rendering
is used when nodes are nested under the .list case in RenderNodeView.
- Around line 347-360: styledShape currently treats "stroke" and "strokeBorder"
the same and always calls resolved.stroke(...); split the conditional that
checks node.modifiers so you handle the "stroke" and "strokeBorder" cases
separately: after applying trim to resolved (using AnyShape and modDouble), if
the modifier name is "stroke" call resolved.stroke(color,lineWidth:), but if
it's "strokeBorder" call resolved.strokeBorder(color,lineWidth:) on the concrete
insettable shape before erasing; use dslColor(clean(...)) and modDouble(...) as
currently used to obtain color and width and only wrap the final result in
AnyView when returning from styledShape.
- Around line 365-377: modifierChildren(_:) currently stacks multiple children
in a ZStack which can misleadingly suggest merging contextMenu entries; instead,
ensure contextMenu consumers see each child as a separate view by avoiding
stacking here—change the ZStack/ForEach combination in modifierChildren(_:) to
render children in a non-stacking container (e.g., use a Group or a simple
ForEach that returns multiple views) or update the contextMenu call site to wrap
modifier.children in a non-stacking container; reference RenderNodeView and
modifierChildren(_:) when making the change.
In
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderStyle.swift`:
- Around line 195-207: The dslKeyEquivalent(_:) function currently maps any
unknown token to KeyEquivalent(raw.first), causing multi-character typos like
"retun" to become a single-character shortcut; change the default branch so it
only returns a KeyEquivalent when the cleaned token (raw) is exactly one
character long (e.g., check raw.count == 1 and use KeyEquivalent(raw.first!));
otherwise return nil, ensuring only single-character tokens produce a
KeyEquivalent and multi-character/unknown tokens are rejected.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 86b64b72-fec8-4d9e-9d11-6e5c4b09a695
📒 Files selected for processing (28)
Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/Environment.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/ParsedProgram.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderModifier.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderNode.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-clock-hud-at-a-glance.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-design-system-shape-grid.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-keyboard-first-workspace-index.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-notifications-activity-control-room.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-port-server-dashboard.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-two-column-cockpit-sidebar.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-concentric-trim-orrery-art.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-keyboard-first-dense-index.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-needs-attention-triage-feed.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-ports-services-health-matrix.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-pr-review-queue-gauge-deck.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-workspace-status-board.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/CorpusCoverageTests.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swiftPackages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swiftPackages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderStyle.swiftdocs/custom-sidebars.mddocs/state-engine-design.md
| Still missing: `@State` and the interactive input controls that need it | ||
| (`TextField`, `Toggle`, `Slider`, `Picker`) — buttons/taps that run `cmux(...)` | ||
| work, but two-way-bound editing does not yet; `switch`; custom `struct`/`View` | ||
| definitions; `gradients` (`LinearGradient`/…); navigation (`sheet`/`popover`/ | ||
| `NavigationStack`); `.keyboardShortcut`; `AsyncImage`/`.resizable`. Workspace | ||
| data (git branch/dirty, ports, PR, unread, remote, latest agent/prompt messages) |
There was a problem hiding this comment.
“Still missing” list is stale against current interpreter behavior.
Line 237 (switch), Line 238 (gradients), and Line 239 (.resizable) are documented as unsupported, but this PR’s shipped surface/tests cover them. This will misdirect sidebar authors and understate available functionality.
Please update this block to keep the authoring contract consistent with the implemented feature set.
🤖 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 `@docs/custom-sidebars.md` around lines 235 - 240, The "Still missing" list is
out of date; update docs/custom-sidebars.md to reflect current interpreter
capabilities by removing or reclassifying items that are now supported
(specifically remove or mark as implemented: switch, gradients/LinearGradient,
and .resizable) and ensure remaining entries still accurately list unsupported
features (e.g., `@State`, TextField, Toggle, Slider, Picker two-way binding,
custom struct/View definitions, navigation APIs like
sheet/popover/NavigationStack, .keyboardShortcut, AsyncImage/.resizable usage
nuances, and workspace data items such as git
branch/dirty/ports/PR/unread/remote/latest agent/prompt messages); keep
references to cmux(...) behavior and any interactive control limitations and
make the wording consistent with shipped tests and surface.
| final class RecursionBudget { | ||
| private(set) var depth = 0 | ||
| private let limit: Int | ||
|
|
||
| /// - Parameter limit: Maximum interpreter nesting depth. The default (400) | ||
| /// is far beyond any legitimate sidebar yet well under the native stack | ||
| /// limit, so deep-but-finite trees still render while infinite recursion | ||
| /// is cut off. | ||
| init(limit: Int = 400) { | ||
| self.limit = limit | ||
| } | ||
|
|
||
| /// Records entry into one more nesting level. | ||
| func enter() { | ||
| depth += 1 | ||
| } | ||
|
|
||
| /// Records exit from a nesting level. | ||
| func leave() { | ||
| if depth > 0 { depth -= 1 } | ||
| } | ||
|
|
||
| /// Whether nesting has passed the limit; callers should bail when true. | ||
| var exceeded: Bool { | ||
| depth > limit | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document thread-safety assumption for the budget lifecycle.
RecursionBudget is a reference type with mutable state (depth) shared across an Environment chain. The implementation is safe because each budget is scoped to a single evaluation session running on a dedicated worker thread (via onLargeStack), but this assumption is not documented.
Consider adding a comment clarifying that concurrent access is not supported and each instance is bound to a single evaluation thread.
📝 Suggested documentation addition
/// A shared recursion-depth counter that bounds interpreter nesting so
/// pathological or malicious authored source degrades to a truncated render
/// instead of overflowing the stack and crashing the host.
///
/// One instance is created at the root ``Environment`` and shared with every
/// child scope. Recursive evaluation entry points call ``enter()`` on the way
/// in (always paired with ``leave()`` via `defer`) and bail when ``exceeded``.
+///
+/// Thread-safety: Each budget instance is scoped to a single evaluation session
+/// on one worker thread; concurrent access to the same budget is not supported.
final class RecursionBudget {🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 8-8: Classes should have an explicit deinit method
(required_deinit)
🤖 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 `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swift`
around lines 8 - 34, Add a clarifying doc comment to RecursionBudget stating
that instances are not thread-safe and are intended to be used only on a single
evaluation/worker thread (e.g., bound to the onLargeStack evaluation session) so
that mutable state (depth) can be shared across an Environment chain without
concurrent access; refer to the class name RecursionBudget and its mutable
properties/methods (depth, enter(), leave(), exceeded, init(limit:)) so
maintainers know the lifecycle and concurrency assumptions.
| /// Runs `work` on a dedicated 16 MB-stack worker thread and returns its | ||
| /// result, so deep recursive-descent parsing and tree-walking do not | ||
| /// overflow the (small) caller stack. | ||
| private func onLargeStack<T: Sendable>(_ work: @escaping @Sendable () -> T) -> T { | ||
| // `box` is written once on the worker and read only after the join | ||
| // signal below, so the unchecked Sendable is safe. | ||
| let box = LargeStackResultBox<T>() | ||
| let done = DispatchSemaphore(value: 0) // one-shot thread-join signal, not a state lock. | ||
| let worker = Thread { | ||
| box.value = work() | ||
| done.signal() | ||
| } | ||
| worker.stackSize = 16 * 1024 * 1024 | ||
| worker.start() | ||
| done.wait() | ||
| return box.value! | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Verify DispatchSemaphore usage for thread-join in synchronous API.
onLargeStack uses DispatchSemaphore.wait() to join the worker thread. While the coding guidelines flag semaphores in production code, this use case differs from typical async coordination:
- The semaphore acts as a pthread_join equivalent for CPU-bound synchronous work (parsing/evaluation).
- The interpreter's public API is synchronous by design (Foundation-only package, no async surface).
- Making it async would require major API changes and force all callers to adopt async.
- The blocking occurs only on the calling thread, which expects synchronous execution.
This pattern is reasonable for a synchronous API that isolates stack-intensive work on a dedicated thread. However, confirm this aligns with the project's concurrency strategy, especially if the caller might be on the main thread.
Based on coding guidelines: Swift code should avoid DispatchSemaphore for blocking waits in production code.
🤖 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 `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift`
around lines 85 - 101, The DispatchSemaphore-based join in onLargeStack (uses
done and worker) blocks the caller thread which contradicts the project's
guideline to avoid blocking waits; either replace the semaphore join with a
higher-level sync primitive (e.g., run the work on an Operation/BlockOperation
and call operationQueue.waitUntilAllOperationsAreFinished()) to avoid manual
semaphores, or if keeping the current Thread + DispatchSemaphore approach, add a
clear comment and a defensive precondition to prevent calling on the main thread
(e.g., assert(!Thread.isMainThread)) and document the rationale in onLargeStack
so it aligns with the synchronous API and project concurrency policy. Ensure
references to LargeStackResultBox<T>, done, and worker remain consistent when
making the change.
| .overlay(alignment: .center) { | ||
| Image(systemName: glyph) | ||
| .font(.system(size: 9)) | ||
| .symbolRenderingMode(.hierarchical) | ||
| .foregroundColor("#0D0D14") | ||
| .opacity(0.0) |
There was a problem hiding this comment.
The status glyph is permanently hidden.
opacity(0.0) makes the centered symbol overlay invisible, so this card never shows the glyph computed at Line 48.
Suggested fix
.overlay(alignment: .center) {
Image(systemName: glyph)
.font(.system(size: 9))
.symbolRenderingMode(.hierarchical)
.foregroundColor("`#0D0D14`")
- .opacity(0.0)
+ .opacity(0.9)
}🤖 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
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swift`
around lines 119 - 124, The overlayed status glyph is permanently hidden because
Image(systemName: glyph) has .opacity(0.0); update the overlay so the glyph is
visible (e.g., remove the opacity modifier or set it to 1.0) so the computed
glyph (glyph) rendered in the .overlay block actually appears; locate the
overlay containing Image(systemName: glyph) and adjust or remove the
.opacity(...) call accordingly.
| case .section: | ||
| VStack(alignment: .leading, spacing: 4) { | ||
| if let header = node.text, !header.isEmpty { | ||
| Text(header) | ||
| .font(.caption) | ||
| .fontWeight(.semibold) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| children | ||
| } | ||
| .frame(maxWidth: .infinity, alignment: .leading) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift"
test -f "$FILE"
echo "== Snippet around case .section =="
rg -n "case \.section" "$FILE"
sed -n '1,140p' "$FILE" | nl -ba | sed -n '1,140p'
echo "== Find unevenRoundedRectangle / strokeBorder / contextMenu usage =="
rg -n "unevenRoundedRectangle|strokeBorder|contextMenu|modifierChildren|Section *\{" "$FILE"
echo "== Print blocks for helpers + modifierChildren =="
# Show likely helper implementations
rg -n "func (styledImage|gradientColors|styledShape|modifierChildren|modDouble|angleDegrees|applyClipShape)" "$FILE" || true
# Print a window around each relevant match
for pat in "unevenRoundedRectangle" "strokeBorder" "modifierChildren" "contextMenu" "styledShape" "applyClipShape"; do
echo "---- window: $pat ----"
line=$(rg -n "$pat" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-40)); end=$((line+80))
sed -n "${start},${end}p" "$FILE" | nl -ba | sed -n '1,200p'
fi
doneRepository: manaflow-ai/cmux
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift"
test -f "$FILE"
echo "== Snippet around case .section =="
rg -n "case \.section" "$FILE"
start=1; end=120
sed -n "${start},${end}p" "$FILE" | cat -n
echo "== Matches for unevenRoundedRectangle / strokeBorder / contextMenu / modifierChildren / Section builder =="
rg -n "unevenRoundedRectangle|strokeBorder|contextMenu|modifierChildren|Section *\{" "$FILE" || true
echo "== Print windows around each match (up to ~60 lines each) =="
for pat in "unevenRoundedRectangle" "strokeBorder" "contextMenu" "modifierChildren" "Section {" ; do
echo "---- window: $pat ----"
# take first match line for each pattern (enough to validate semantics)
line=$(rg -n "$pat" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-25)); end=$((line+60))
if [ $start -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" "$FILE" | cat -n
else
echo "(no match found)"
fi
done
echo "== Show the full section-case body only (from case .section line to next case) =="
secLine=$(rg -n "case \.section" "$FILE" | head -n1 | cut -d: -f1)
# find next "case ." after secLine
nextLine=$(awk -v s="$secLine" 'NR>s && $0 ~ /^[[:space:]]*case \./ {print NR; exit}' "$FILE")
if [ -z "${nextLine:-}" ]; then nextLine=$((secLine+80)); fi
start=$secLine
end=$((nextLine-1))
sed -n "${start},${end}p" "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 26713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "unevenRoundedRectangle" -S . | head -n 50
echo "---- Search for RenderNode kind definition / corner radius fields ----"
# try to locate RenderNode definition
rg -n "struct RenderNode|enum RenderNode|typealias RenderNode|RenderNode\.Kind|enum Kind|case \.unevenRoundedRectangle" -S Packages | head -n 80
echo "---- Search for cornerRadius property on RenderNode ----"
rg -n "cornerRadius" Packages | head -n 80Repository: manaflow-ai/cmux
Length of output: 14569
Render .section as a real SwiftUI Section inside List.
RenderNodeView currently renders case .section as a VStack (header + children), so when .section nodes appear under case .list they become a single list row and lose SwiftUI section/list semantics (header behavior, accessibility, and styling).
Proposed fix
- case .section:
- VStack(alignment: .leading, spacing: 4) {
- if let header = node.text, !header.isEmpty {
- Text(header)
- .font(.caption)
- .fontWeight(.semibold)
- .foregroundStyle(.secondary)
- }
- children
- }
- .frame(maxWidth: .infinity, alignment: .leading)
+ case .section:
+ Section {
+ children
+ } header: {
+ if let header = node.text, !header.isEmpty {
+ Text(header)
+ .font(.caption)
+ .fontWeight(.semibold)
+ .foregroundStyle(.secondary)
+ }
+ }🤖 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
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`
around lines 49 - 59, RenderNodeView currently uses a VStack for case .section
which breaks SwiftUI List semantics; change the .section branch to emit a
SwiftUI Section when it may appear inside a List: replace the VStack with
Section(header: { if let header = node.text, !header.isEmpty {
Text(header).font(.caption).fontWeight(.semibold).foregroundStyle(.secondary) }
}, content: { children }) so the header and children are actual Section parts
(preserving the same Text styling and alignment) and remove the manual .frame
used for the VStack; keep the existing children view and ensure this rendering
is used when nodes are nested under the .list case in RenderNodeView.
| case .ellipse: | ||
| styledShape(Ellipse()) | ||
| case .unevenRoundedRectangle: | ||
| styledShape(RoundedRectangle(cornerRadius: CGFloat(node.cornerRadius ?? 6))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="."
# 1) Inspect the target file around the reported lines
target="Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift"
echo "==== $target (relevant section) ===="
sed -n '80,150p' "$target" | cat -n
# 2) Search for the node kind and any existing per-corner radii usage
echo "==== rg unevenRoundedRectangle ===="
rg -n "unevenRoundedRectangle" Packages | head -n 50
# 3) Search for cornerRadii / UnevenRoundedRectangle usage
echo "==== rg cornerRadii / UnevenRoundedRectangle ===="
rg -n "cornerRadii|UnevenRoundedRectangle" Packages | head -n 50
# 4) Find RenderNode definition and inspect what radii it carries
echo "==== rg \"struct RenderNode|class RenderNode|enum RenderNode|typealias RenderNode\" ===="
rg -n "(struct|class|enum|typealias) RenderNode" Packages/CmuxSwiftRenderUI | head -n 50
# 5) Also search for "cornerRadius" / "cornerRadii" in RenderNode-related files
echo "==== rg \"cornerRadius|cornerRadii\" in RenderNode-related area ===="
rg -n "cornerRadius|cornerRadii" Packages/CmuxSwiftRenderUI | head -n 100Repository: manaflow-ai/cmux
Length of output: 7858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== Inspect RenderNode.swift ===="
sed -n '1,120p' Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderNode.swift | cat -n
echo "==== Inspect SwiftViewInterpreter.swift around UnevenRoundedRectangle ===="
sed -n '150,260p' Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift | cat -n
echo "==== rg topLeading/bottomLeading/topTrailing/bottomTrailing in CmuxSwiftRender ===="
rg -n "topLeading|bottomLeading|topTrailing|bottomTrailing|cornerRadii|cornerRadius(s)?:" Packages/CmuxSwiftRender/Sources/CmuxSwiftRender | head -n 200Repository: manaflow-ai/cmux
Length of output: 14190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== Inspect RenderNode.swift ===="
sed -n '1,120p' Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderNode.swift | cat -n
echo "==== Inspect SwiftViewInterpreter.swift around UnevenRoundedRectangle ===="
sed -n '150,260p' Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swift | cat -n
echo "==== rg topLeading/bottomLeading/topTrailing/bottomTrailing/cornerRadii in CmuxSwiftRender ===="
rg -n "topLeading|bottomLeading|topTrailing|bottomTrailing|cornerRadii" Packages/CmuxSwiftRender/Sources/CmuxSwiftRender | head -n 200Repository: manaflow-ai/cmux
Length of output: 13414
unevenRoundedRectangle is rendered as a uniform RoundedRectangle (cornerRadius-only).
RenderNode.Kind.unevenRoundedRectangle is documented as a uniform “cornerRadius” approximation; RenderNode only stores a single cornerRadius value, and SwiftViewInterpreter only parses cornerRadius (or falls back to topLeadingRadius) into that field. RenderNodeView then renders it via RoundedRectangle(cornerRadius:), flattening any authored per-corner radii.
Extend the IR to carry all four corner radii and lower .unevenRoundedRectangle to SwiftUI UnevenRoundedRectangle(...) using those values instead of reusing RoundedRectangle.
🤖 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
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`
around lines 114 - 117, RenderNode.Kind.unevenRoundedRectangle is currently
flattened to a uniform RoundedRectangle because RenderNode only stores a single
cornerRadius; update the IR and lowering so unevenRoundedRectangle carries four
corner radii (topLeading, topTrailing, bottomLeading, bottomTrailing), modify
SwiftViewInterpreter to parse all four radii into RenderNode (falling back to
the existing cornerRadius when per-corner values are absent), and change
RenderNodeView's case for .unevenRoundedRectangle (and the styledShape call) to
produce SwiftUI UnevenRoundedRectangle(...) using those four radii instead of
RoundedRectangle(cornerRadius:).
| /// Renders a shape, applying shape-level `.trim` then `.stroke` / | ||
| /// `.strokeBorder` (which must act on the concrete `Shape` before erasure). | ||
| /// With no stroke the plain shape is returned so `.fill`/`.foregroundColor` | ||
| /// from the generic modifier pass fills it. | ||
| private func styledShape(_ shape: some Shape) -> AnyView { | ||
| var resolved = AnyShape(shape) | ||
| if let trim = node.modifiers.first(where: { $0.name == "trim" }) { | ||
| resolved = AnyShape(resolved.trim(from: CGFloat(modDouble(trim, "from") ?? 0), | ||
| to: CGFloat(modDouble(trim, "to") ?? 1))) | ||
| } | ||
| if let stroke = node.modifiers.first(where: { $0.name == "stroke" || $0.name == "strokeBorder" }) { | ||
| let color = dslColor(clean(stroke.firstValue)) ?? .secondary | ||
| let width = modDouble(stroke, "lineWidth") ?? 1 | ||
| return AnyView(resolved.stroke(color, lineWidth: CGFloat(width))) |
There was a problem hiding this comment.
Render strokeBorder with strokeBorder(...), not stroke(...) (RenderNodeView.swift:347-360)
styledShape(_:) combines "stroke" and "strokeBorder" into one branch and always returns resolved.stroke(color, lineWidth: ...), so strokeBorder doesn’t stay inside the insettable shape bounds. Split the branches by modifier name and call resolved.strokeBorder(color, lineWidth: ...) for "strokeBorder" (after applying .trim, before erasing to AnyView).
🤖 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
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`
around lines 347 - 360, styledShape currently treats "stroke" and "strokeBorder"
the same and always calls resolved.stroke(...); split the conditional that
checks node.modifiers so you handle the "stroke" and "strokeBorder" cases
separately: after applying trim to resolved (using AnyShape and modDouble), if
the modifier name is "stroke" call resolved.stroke(color,lineWidth:), but if
it's "strokeBorder" call resolved.strokeBorder(color,lineWidth:) on the concrete
insettable shape before erasing; use dslColor(clean(...)) and modDouble(...) as
currently used to obtain color and width and only wrap the final result in
AnyView when returning from styledShape.
| /// Renders a child-bearing modifier's subtree (overlay/background/mask | ||
| /// content). Multiple top-level views stack in a `ZStack`. | ||
| @ViewBuilder | ||
| private func modifierChildren(_ modifier: RenderModifier) -> some View { | ||
| if modifier.children.count == 1 { | ||
| RenderNodeView(node: modifier.children[0]) | ||
| } else { | ||
| ZStack { | ||
| ForEach(Array(modifier.children.enumerated()), id: \.offset) { _, child in | ||
| RenderNodeView(node: child) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Don’t treat modifierChildren(_:) as collapsing contextMenu entries.
contextMenu is built from modifier.children, and modifierChildren(_:) renders one RenderNodeView per child (via the ForEach), so wrapping those views in a ZStack shouldn’t merge multiple authored buttons into a single menu item. If desired, swap the contextMenu call site to a non-stacking container for readability/semantics, but this isn’t a correctness issue.
🤖 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
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`
around lines 365 - 377, modifierChildren(_:) currently stacks multiple children
in a ZStack which can misleadingly suggest merging contextMenu entries; instead,
ensure contextMenu consumers see each child as a separate view by avoiding
stacking here—change the ZStack/ForEach combination in modifierChildren(_:) to
render children in a non-stacking container (e.g., use a Group or a simple
ForEach that returns multiple views) or update the contextMenu call site to wrap
modifier.children in a non-stacking container; reference RenderNodeView and
modifierChildren(_:) when making the change.
| func dslKeyEquivalent(_ token: String?) -> KeyEquivalent? { | ||
| guard let raw = token?.trimmingCharacters(in: CharacterSet(charactersIn: ".\" ")), !raw.isEmpty else { return nil } | ||
| switch raw.lowercased() { | ||
| case "return": return .return | ||
| case "escape": return .escape | ||
| case "space": return .space | ||
| case "tab": return .tab | ||
| case "delete": return .delete | ||
| case "uparrow": return .upArrow | ||
| case "downarrow": return .downArrow | ||
| case "leftarrow": return .leftArrow | ||
| case "rightarrow": return .rightArrow | ||
| default: return raw.first.map { KeyEquivalent($0) } |
There was a problem hiding this comment.
Fix dslKeyEquivalent to reject multi-character/unknown shortcut tokens
dslKeyEquivalent(_:) currently maps any unknown non-empty token to raw.first, so typos like "retun" silently become a shortcut for "r" instead of returning nil. In Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderStyle.swift (lines 195-207), update the default branch to only return a KeyEquivalent when the cleaned token is exactly one character.
Proposed fix
switch raw.lowercased() {
case "return": return .return
case "escape": return .escape
case "space": return .space
case "tab": return .tab
case "delete": return .delete
case "uparrow": return .upArrow
case "downarrow": return .downArrow
case "leftarrow": return .leftArrow
case "rightarrow": return .rightArrow
- default: return raw.first.map { KeyEquivalent($0) }
+ default:
+ guard raw.count == 1, let character = raw.first else { return nil }
+ return KeyEquivalent(character)
}
}🤖 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
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderStyle.swift`
around lines 195 - 207, The dslKeyEquivalent(_:) function currently maps any
unknown token to KeyEquivalent(raw.first), causing multi-character typos like
"retun" to become a single-character shortcut; change the default branch so it
only returns a KeyEquivalent when the cleaned token (raw) is exactly one
character long (e.g., check raw.count == 1 and use KeyEquivalent(raw.first!));
otherwise return nil, ensuring only single-character tokens produce a
KeyEquivalent and multi-character/unknown tokens are rejected.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift (1)
88-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport button targets after
applyModifiers(...).These two
.reportTapTarget(node.action)calls run insidecontent, butbodyappliesnode.modifiersafterward. Any button with outer layout/transform modifiers will report the pre-modifier rect instead of the actual tappable bounds. Move button reporting intobodyso it wraps the finalview, the same way the non-button gesture path does.Suggested fix
- .reportTapTarget(node.action) } else { // Rich label form: `Button(action:){ label }`. Plain style so // the label renders as authored, not as default button chrome. Button { if let action = node.action { dispatch.run(action) } @@ } .buttonStyle(.plain) - .reportTapTarget(node.action) }var body: some View { let view = applyModifiers(content, node.modifiers) if let action = node.action { if node.kind == .button { return AnyView(view.reportTapTarget(action)) } return AnyView( view.contentShape(Rectangle()) .onTapGesture { dispatch.run(action) } .reportTapTarget(action) ) } return view }🤖 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 `@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift` around lines 88 - 103, The button tap-target reporting is currently attached inside the Button label branches so it records pre-modifier bounds; instead move/report after applyModifiers(...) in the view body so the final transformed view is wrapped. Modify RenderNodeView.body to build the base view via let view = applyModifiers(content, node.modifiers) and then, if let action = node.action, wrap that view with .reportTapTarget(action) for node.kind == .button (and for non-button use contentShape + onTapGesture + .reportTapTarget) before returning; update references to node.kind, node.action, applyModifiers, reportTapTarget and dispatch.run accordingly so both Button and non-button gesture paths report post-modifier rects.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In
`@Packages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift`:
- Around line 88-103: The button tap-target reporting is currently attached
inside the Button label branches so it records pre-modifier bounds; instead
move/report after applyModifiers(...) in the view body so the final transformed
view is wrapped. Modify RenderNodeView.body to build the base view via let view
= applyModifiers(content, node.modifiers) and then, if let action = node.action,
wrap that view with .reportTapTarget(action) for node.kind == .button (and for
non-button use contentShape + onTapGesture + .reportTapTarget) before returning;
update references to node.kind, node.action, applyModifiers, reportTapTarget and
dispatch.run accordingly so both Button and non-button gesture paths report
post-modifier rects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b200594f-34da-4ea3-8165-3d6fb76027a2
📒 Files selected for processing (4)
Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderModifier.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderNode.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swiftPackages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swift
…tal: Int(1.0 / 0.0) in authored sidebar source crashes the interpreting process (Swift's Int(_: Double) traps on non-finite/overflow), and Gauge(value:total:) renders full because only the raw value: is captured. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Int(exactly:) on the truncated value returns nil for NaN/infinity/overflow instead of trapping, so authored source like Int(1.0 / 0.0) renders best-effort rather than killing the interpreting process. Gauge(value:total:) now normalizes through the same progressValue logic as ProgressView, so total-relative gauges render proportionally instead of pinned full. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swift (1)
640-644:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name/comment claims non-finite but uses finite value.
The test name and comment indicate testing non-finite values (
.infinity,.nan) but Line 642 uses2.0(finite, just out-of-range). This doesn't validate the non-finite crash path described.💡 Proposed fix
- let node = interp.evaluate(#"VStack { Rectangle().foregroundColor(Color(red: 2.0, green: 0.5, blue: 0.0)) ; Text("ok") }"#) + let node = interp.evaluate(#"VStack { Rectangle().foregroundColor(Color(red: .infinity, green: 0.5, blue: 0.0)) ; Text("ok") }"#)Alternatively, if you intended to test out-of-range (not non-finite), rename the test to
colorChannelHandlesOutOfRangeWithoutCrashingand update the comment.🤖 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 `@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swift` around lines 640 - 644, The test colorChannelHandlesNonFiniteWithoutCrashing currently asserts non-finite handling but passes a finite 2.0 to Color; update the test in SwiftViewInterpreterTests (method colorChannelHandlesNonFiniteWithoutCrashing) so the interp.evaluate call uses a non-finite value (e.g., Color(red: .infinity, green: 0.5, blue: 0.0) or .nan) and update the inline comment to match, or alternatively rename the test and comment to colorChannelHandlesOutOfRangeWithoutCrashing if you intend to test out-of-range finite values.Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swift (1)
8-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument thread-safety assumption for the budget lifecycle.
RecursionBudgetis a reference type with mutable state (depth) shared across anEnvironmentchain. The implementation is safe because each budget is scoped to a single evaluation session running on a dedicated worker thread (viaonLargeStack), but this assumption is not documented.Consider adding a comment clarifying that concurrent access is not supported and each instance is bound to a single evaluation thread.
📝 Suggested documentation addition
/// A shared recursion-depth counter that bounds interpreter nesting so /// pathological or malicious authored source degrades to a truncated render /// instead of overflowing the stack and crashing the host. /// /// One instance is created at the root ``Environment`` and shared with every /// child scope. Recursive evaluation entry points call ``enter()`` on the way /// in (always paired with ``leave()`` via `defer`) and bail when ``exceeded``. +/// +/// Thread-safety: Each budget instance is scoped to a single evaluation session +/// on one worker thread; concurrent access to the same budget is not supported. final class RecursionBudget {🤖 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 `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swift` around lines 8 - 34, Add a short documentation comment to RecursionBudget making its thread-safety assumptions explicit: state that RecursionBudget is a reference type with mutable state (depth) that is not safe for concurrent access and must be used only from a single evaluation/worker thread (e.g., the onLargeStack evaluation session) for its lifetime; mention that enter(), leave(), and exceeded are intended to be called from that single thread only. This comment should be placed on the RecursionBudget type (near the class/initializer) and reference the mutable depth and the enter/leave methods so future maintainers know the lifecycle and concurrency constraints.Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swift (1)
66-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclusive ranges at
Int.maxare dropped incorrectly.Line 68 computes
upper + 1for inclusive ranges, soInt.max...Int.maxhits overflow and returns[]at Line 69 even though it should yield one element. Compute count without formingupper + 1.Suggested fix
case let .range(lower, upper, inclusive): // Overflow-safe end + a materialization cap so a pathological range // (e.g. `0...Int.max`) can't overflow or exhaust memory. - let (end, addOverflow) = inclusive ? upper.addingReportingOverflow(1) : (upper, false) - guard !addOverflow, end >= lower else { return [] } - let (count, subOverflow) = end.subtractingReportingOverflow(lower) - guard !subOverflow, count <= 100_000 else { return [] } - return (lower..<end).map(SwiftValue.int) + guard upper >= lower else { return [] } + let (delta, subOverflow) = upper.subtractingReportingOverflow(lower) + guard !subOverflow else { return [] } + let (count, addOverflow) = inclusive + ? delta.addingReportingOverflow(1) + : (delta, false) + guard !addOverflow, count <= 100_000 else { return [] } + return (0..<count).map { .int(lower + $0) }🤖 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 `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swift` around lines 66 - 72, The current logic overflows by computing upper + 1 for inclusive ranges; instead compute the element count safely as (upper - lower) + (inclusive ? 1 : 0) using subtractingReportingOverflow and addingReportingOverflow so you never form upper+1. Concretely: use upper.subtractingReportingOverflow(lower) to get rawDiff and guard no overflow, then add 1 to rawDiff with addingReportingOverflow when inclusive and guard no overflow and count <= 100_000; finally build the result by producing values from lower using the safe count (e.g. (0..<count).map { SwiftValue.int(lower + $0) }), referencing lower, upper, inclusive and SwiftValue.int to locate and update the code.Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swift (1)
159-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap these literal arrays in
enumerated().
id: \.offsetand the{ i, cell in ... }closure shape expect(offset, element)tuples, but bothForEachsources here are plain[[String]]. These tiles won't match SwiftUI'sForEachcontract as written.Suggested fix
- ForEach([["changes_requested", "\(blocking)"], ["review_required", "\(waiting)"]], id: \.offset) { i, cell in + ForEach(Array([["changes_requested", "\(blocking)"], ["review_required", "\(waiting)"]].enumerated()), id: \.offset) { i, cell in let st = cell[0] VStack(alignment: .leading, spacing: 6) { @@ - ForEach([["approved", "\(ready)"], ["merged", "\(merged)"]], id: \.offset) { i, cell in + ForEach(Array([["approved", "\(ready)"], ["merged", "\(merged)"]].enumerated()), id: \.offset) { i, cell in let st = cell[0] VStack(alignment: .leading, spacing: 6) {🤖 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 `@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swift` around lines 159 - 191, The ForEach calls using id: \.offset and closure signature `{ i, cell in ... }` are iterating plain [[String]] literals; wrap each array literal in .enumerated() so the sequence yields (offset, element) tuples that match the `{ i, cell in }` parameters and the id: \.offset. Update both ForEach invocations (the one with ["changes_requested", "\(blocking)"], ["review_required", "\(waiting)"] and the one with ["approved", "\(ready)"], ["merged", "\(merged)"]) to call .enumerated() on the array so the closure and id work correctly.Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swift (1)
119-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the status glyph visible.
Image(systemName: glyph)is still fully hidden by.opacity(0.0), so the card never renders the computed status icon.Suggested fix
.overlay(alignment: .center) { Image(systemName: glyph) .font(.system(size: 9)) .symbolRenderingMode(.hierarchical) .foregroundColor("`#0D0D14`") - .opacity(0.0) + .opacity(0.9) }🤖 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 `@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swift` around lines 119 - 124, The status glyph is hidden by .opacity(0.0) on the Image(systemName: glyph) in the overlay; update the Image overlay (the Image(systemName: glyph) modifier chain) to make the glyph visible by removing or changing .opacity(0.0) to a visible value (e.g., .opacity(1.0)) so the computed status icon renders.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-needs-attention-triage-feed.swift`:
- Around line 1-6: The review points out that ws.pr and ws.remote are optional
and currently dereferenced unsafely in functions like urgencyScore and UI code
(remoteLabel, header counts, row chips/menu); update all uses (e.g.,
urgencyScore, remoteLabel, header count computations, and row chip/menu logic)
to safely handle optionals by using optional chaining and sensible defaults or
guards—for example replace direct accesses like ws.remote.connected and
ws.pr.stale with safe expressions such as ws.remote?.connected ?? false and
ws.pr?.stale ?? false (or early-return/guard let when you need the full object),
and ensure unread/pinned/dirty computations account for missing pr/remote so the
feed ranks and renders correctly.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-pr-review-queue-gauge-deck.swift`:
- Around line 400-402: The onTapGesture currently dispatches cmux(pr.stale ?
"pr.refresh" : "workspace.select", workspace_id: w.id) which sends a
workspace_id when pr.stale is true but all other refresh paths expect the PR
number; change the stale branch to dispatch the same argument shape as other
refreshes by calling cmux("pr.refresh", pr_number: pr.number) (or the existing
PR argument key used elsewhere) instead of passing workspace_id, keeping the
non-stale branch unchanged.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-workspace-status-board.swift`:
- Around line 211-213: The tap currently calls cmux("workspace.select", value:
w.id) which records the parameter label "value" instead of the required
"workspace_id"; update the call in the ForEach row (the .onTapGesture attached
to boardRow(w)) to use the workspace_id label (e.g. cmux("workspace.select",
workspace_id: w.id)) so the serialized payload matches the expected workspace_id
shape.
---
Duplicate comments:
In `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swift`:
- Around line 8-34: Add a short documentation comment to RecursionBudget making
its thread-safety assumptions explicit: state that RecursionBudget is a
reference type with mutable state (depth) that is not safe for concurrent access
and must be used only from a single evaluation/worker thread (e.g., the
onLargeStack evaluation session) for its lifetime; mention that enter(),
leave(), and exceeded are intended to be called from that single thread only.
This comment should be placed on the RecursionBudget type (near the
class/initializer) and reference the mutable depth and the enter/leave methods
so future maintainers know the lifecycle and concurrency constraints.
In `@Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swift`:
- Around line 66-72: The current logic overflows by computing upper + 1 for
inclusive ranges; instead compute the element count safely as (upper - lower) +
(inclusive ? 1 : 0) using subtractingReportingOverflow and
addingReportingOverflow so you never form upper+1. Concretely: use
upper.subtractingReportingOverflow(lower) to get rawDiff and guard no overflow,
then add 1 to rawDiff with addingReportingOverflow when inclusive and guard no
overflow and count <= 100_000; finally build the result by producing values from
lower using the safe count (e.g. (0..<count).map { SwiftValue.int(lower + $0)
}), referencing lower, upper, inclusive and SwiftValue.int to locate and update
the code.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swift`:
- Around line 119-124: The status glyph is hidden by .opacity(0.0) on the
Image(systemName: glyph) in the overlay; update the Image overlay (the
Image(systemName: glyph) modifier chain) to make the glyph visible by removing
or changing .opacity(0.0) to a visible value (e.g., .opacity(1.0)) so the
computed status icon renders.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swift`:
- Around line 159-191: The ForEach calls using id: \.offset and closure
signature `{ i, cell in ... }` are iterating plain [[String]] literals; wrap
each array literal in .enumerated() so the sequence yields (offset, element)
tuples that match the `{ i, cell in }` parameters and the id: \.offset. Update
both ForEach invocations (the one with ["changes_requested", "\(blocking)"],
["review_required", "\(waiting)"] and the one with ["approved", "\(ready)"],
["merged", "\(merged)"]) to call .enumerated() on the array so the closure and
id work correctly.
In
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swift`:
- Around line 640-644: The test colorChannelHandlesNonFiniteWithoutCrashing
currently asserts non-finite handling but passes a finite 2.0 to Color; update
the test in SwiftViewInterpreterTests (method
colorChannelHandlesNonFiniteWithoutCrashing) so the interp.evaluate call uses a
non-finite value (e.g., Color(red: .infinity, green: 0.5, blue: 0.0) or .nan)
and update the inline comment to match, or alternatively rename the test and
comment to colorChannelHandlesOutOfRangeWithoutCrashing if you intend to test
out-of-range finite values.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: be16c53e-3979-41e7-8629-76eb4316719b
📒 Files selected for processing (28)
Packages/CmuxSwiftRender/Sources/CmuxSwiftRender/Environment.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/ExpressionEvaluator.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/ParsedProgram.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RecursionBudget.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderModifier.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/RenderNode.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftValue.swiftPackages/CmuxSwiftRender/Sources/CmuxSwiftRender/SwiftViewInterpreter.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-agent-control-room.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-clock-hud-at-a-glance.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-design-system-shape-grid.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-git-review-queue-command-deck.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-keyboard-first-workspace-index.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-notifications-activity-control-room.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-port-server-dashboard.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress-two-column-cockpit-sidebar.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-concentric-trim-orrery-art.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-keyboard-first-dense-index.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-needs-attention-triage-feed.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-ports-services-health-matrix.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-pr-review-queue-gauge-deck.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-workspace-status-board.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/CorpusCoverageTests.swiftPackages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swiftPackages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderNodeView.swiftPackages/CmuxSwiftRenderUI/Sources/CmuxSwiftRenderUI/Rendering/RenderStyle.swiftdocs/custom-sidebars.mddocs/state-engine-design.md
| func urgencyScore(_ ws: Workspace) -> Int { | ||
| let downPenalty = ws.remote.connected ? 0 : 60 | ||
| let prPenalty = ws.pr.stale ? 25 : 0 | ||
| let dirtyPenalty = ws.dirty ? 8 : 0 | ||
| let pinBonus = ws.pinned ? 5 : 0 | ||
| return ws.unread * 4 + downPenalty + prPenalty + dirtyPenalty + pinBonus |
There was a problem hiding this comment.
Treat pr and remote as optional here too.
urgencyScore, remoteLabel, the header counts, and the row chips/menu all dereference ws.pr / ws.remote without guards. On local workspaces or ones without an attached PR, this feed can mis-rank items or fail to render entirely.
Also applies to: 44-47, 73-75, 123-131, 194-199, 257-258
🤖 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
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-needs-attention-triage-feed.swift`
around lines 1 - 6, The review points out that ws.pr and ws.remote are optional
and currently dereferenced unsafely in functions like urgencyScore and UI code
(remoteLabel, header counts, row chips/menu); update all uses (e.g.,
urgencyScore, remoteLabel, header count computations, and row chip/menu logic)
to safely handle optionals by using optional chaining and sensible defaults or
guards—for example replace direct accesses like ws.remote.connected and
ws.pr.stale with safe expressions such as ws.remote?.connected ?? false and
ws.pr?.stale ?? false (or early-return/guard let when you need the full object),
and ensure unread/pinned/dirty computations account for missing pr/remote so the
feed ranks and renders correctly.
| .onTapGesture { | ||
| cmux(pr.stale ? "pr.refresh" : "workspace.select", workspace_id: w.id) | ||
| } |
There was a problem hiding this comment.
Refresh-on-tap sends the wrong argument shape for stale PRs.
The stale branch dispatches pr.refresh with workspace_id, while every other refresh path in this file uses the PR number. Tapping a stale card will capture a different command than the menu and likely won't refresh the intended PR.
Suggested fix
- .onTapGesture {
- cmux(pr.stale ? "pr.refresh" : "workspace.select", workspace_id: w.id)
- }
+ .onTapGesture {
+ if pr.stale {
+ cmux("pr.refresh", number: pr.number)
+ } else {
+ cmux("workspace.select", workspace_id: w.id)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .onTapGesture { | |
| cmux(pr.stale ? "pr.refresh" : "workspace.select", workspace_id: w.id) | |
| } | |
| .onTapGesture { | |
| if pr.stale { | |
| cmux("pr.refresh", number: pr.number) | |
| } else { | |
| cmux("workspace.select", workspace_id: w.id) | |
| } | |
| } |
🤖 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
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-pr-review-queue-gauge-deck.swift`
around lines 400 - 402, The onTapGesture currently dispatches cmux(pr.stale ?
"pr.refresh" : "workspace.select", workspace_id: w.id) which sends a
workspace_id when pr.stale is true but all other refresh paths expect the PR
number; change the stale branch to dispatch the same argument shape as other
refreshes by calling cmux("pr.refresh", pr_number: pr.number) (or the existing
PR argument key used elsewhere) instead of passing workspace_id, keeping the
non-stale branch unchanged.
| ForEach(workspaces) { w in | ||
| boardRow(w) | ||
| .onTapGesture { cmux("workspace.select", value: w.id) } |
There was a problem hiding this comment.
Use workspace_id for workspace.select.
ActionCommand.cmux records parameter labels verbatim, so this currently serializes {"value": ...} instead of the workspace_id shape used everywhere else for workspace.select. The row tap can be captured but not dispatched correctly by the host.
Suggested fix
- boardRow(w)
- .onTapGesture { cmux("workspace.select", value: w.id) }
+ boardRow(w)
+ .onTapGesture { cmux("workspace.select", workspace_id: w.id) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ForEach(workspaces) { w in | |
| boardRow(w) | |
| .onTapGesture { cmux("workspace.select", value: w.id) } | |
| ForEach(workspaces) { w in | |
| boardRow(w) | |
| .onTapGesture { cmux("workspace.select", workspace_id: w.id) } |
🤖 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
`@Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/Corpus/stress2-workspace-status-board.swift`
around lines 211 - 213, The tap currently calls cmux("workspace.select", value:
w.id) which records the parameter label "value" instead of the required
"workspace_id"; update the call in the ForEach row (the .onTapGesture attached
to boardRow(w)) to use the workspace_id label (e.g. cmux("workspace.select",
workspace_id: w.id)) so the serialized payload matches the expected workspace_id
shape.
Dismissing per maintainer instruction: bot verdicts advisory; all CI checks including the full tests job pass on the final head.
Implements the leaf-tier (and language-tier) capability matrix from
docs/swiftui-interpreter-surface.md, pushing the runtime SwiftUI interpreter toward "interpret any SwiftUI". Stacked on #5254 (basefeat-dsl-sidebar); retarget base tomainonce #5254 merges.Every batch is verified by the
CorpusCoverageTestsharness + behavior tests. 56 interpreter tests pass; all 28 corpus sidebars render (0 NIL); full app builds.What landed
Views/containers: List, Section, LazyVStack/LazyHStack, Group/EmptyView, Grid/GridRow, LazyVGrid/LazyHGrid, ViewThatFits, horizontal ScrollView, Label, Menu, ProgressView, Gauge, AnyView passthrough, Ellipse/UnevenRoundedRectangle.
Modifiers: full text/typography (italic/monospaced/fontDesign/tracking-family/textCase/truncationMode/underline/…), layout (offset/zIndex/aspectRatio/fixedSize/layoutPriority/scaledToFit-Fill), decoration (shadow/border/blur/clipShape/brightness/contrast/saturation/grayscale/rotationEffect/scaleEffect/redacted), SF-symbol (imageScale/symbolRenderingMode/symbolVariant), interaction (contextMenu/help/disabled/keyboardShortcut/accessibility), and the arbitrary-child composability keystone:
.overlay{}/.background{}/.mask{}/.safeAreaInset{}accept nested views.Shapes & color: shape
.stroke/.strokeBorder/.trim(applied on the concrete shape via AnyShape before erasure), expanded color palette, and Linear/Radial/Angular gradients (usable standalone and inside.background{}).Language tier:
if let/guard letoptional binding andswitch(both in view position and inside user value funcs), plus value methods (enumerated/indices/dropFirst/dropLast/suffix/min/max/abs/Int()/Double()/String()).Image:
.resizable().Robustness (important)
evaluate()runs parse+interpret on a 16 MB-stack worker Thread with a per-EnvironmentRecursionBudgetbackstop — deeply-nested or pathological authored source renders best-effort instead of overflowing the stack (fixed a SIGBUS found by the stress corpus). An interpreter for untrusted authored source must never crash cmux.Process
Two multi-agent stress rounds authored 14 ambitious composed sidebars (added to the corpus as permanent fixtures); the harness drove implementation to close every gap they surfaced. Authoring guide (
docs/custom-sidebars.md) and thecmux-swift-interpreterskill updated to the full surface.Not in this PR (designed, follow-up)
@State+ two-way bindings + input controls (TextField/Toggle/Slider/Picker) — the interactivity tier — is designed indocs/state-engine-design.mdbut intentionally not built here, since interactive behavior needs real dogfooding (typing/toggling) that a non-interactive build pass can't verify. Also deferred: AsyncImage/URL (async/file), navigation/presentation.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation