Repository navigation
Add a Lisp for customizing the sidebar - #5177
lawrencecchen wants to merge 13 commits into
Conversation
New package CmuxSidebarScript: a small Scheme-ish Lisp whose render-row function receives one workspace's data and returns a view tree, rendered as SwiftUI. Users drop a script in ~/.config/cmux/sidebar.lisp; with no file the sidebar renders natively as before, so it is fully opt-in. - Reader, evaluator (scopes, closures, step/depth budget), standard library. - SwiftUI bridge: views and ~30 :keyword modifiers, easy to extend. - Pure Equatable RenderNode tree rendered by RenderNodeView; keeps the row .equatable() perf contract and makes evaluation host-free testable. - DefaultSidebar.lisp reproduces a real row (title, pin, unread, branch, directory, PRs, ports, progress). Integration in TabItemView: when a script is active the row content renders through RenderNodeView while cmux keeps selection chrome, drag, tap, and context menu. The script is passed as an immutable value plus a version folded into ==, preserving the snapshot-boundary and typing-latency contracts. Compile or render faults log and fall back to the native row. Linked into cmux and cmux-unit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CmuxSidebarScript: a packaged Scheme-like reader/evaluator with stdlib, a pure RenderNode model and RNValue types, a Lisp→Render bridge, SwiftUI renderer, default sidebar script resource, tests, and host integration (SidebarScriptStore + ContentView wiring). ChangesSidebar Script Runtime and Rendering
Integration into main app: SidebarScriptStore and ContentView
Sequence Diagram(s)sequenceDiagram
participant Reader
participant Evaluator
participant Env as LispEnvironment
participant Bridge
participant Render as RenderNodeModel
participant SwiftUI
Reader->>Evaluator: parsed forms
Evaluator->>Env: define/top-level binds
Bridge->>Env: install constructors & builtins
Evaluator->>Render: produce RenderNode via bridge constructors
Render->>SwiftUI: RenderNodeView renders SwiftUI views
SwiftUI->>App: ContentView uses SidebarScriptStore to supply script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e5737e62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .testTarget( | ||
| name: "CmuxSidebarScriptTests", | ||
| dependencies: ["CmuxSidebarScript"], |
There was a problem hiding this comment.
This creates a SwiftPM test target, but the checked .github/workflows/ci.yml “Run Swift package unit tests” step only enumerates CmuxFoundation, CmuxSettings, CmuxSettingsUI, and CmuxSocketControl, and that workflow explicitly notes cmux-unit does not execute SPM package test targets. Unless this package is added to that PACKAGES list or included in an Xcode scheme, the new reader/evaluator/bridge tests are not a CI gate and regressions in this new scripting engine can merge untested.
Useful? React with 👍 / 👎.
| if step > 0 { while x < end { out.append(.int(Int(x))); x += step } } | ||
| else { while x > end { out.append(.int(Int(x))); x += step } } |
There was a problem hiding this comment.
When a user script calls (range 1000000000) or accidentally uses the exposed infinity constant as the end value, this loop runs synchronously during sidebar row rendering and appends without consulting Evaluator.stepLimit, so the app can hang or OOM before the render fallback ever runs. Please cap the number of emitted elements or check/consume the evaluator budget inside this loop.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds
Confidence Score: 3/5Two blocking main-thread I/O paths in the new render and state-mutation code — one of which scales with workspace count — need to be moved off @mainactor before the render-sidebar path is usable at production scale. The package-level engine (reader, evaluator, bridge, RenderNode) is well-structured and its correctness boundaries are solid. The integration layer introduces two new synchronous filesystem operations on the main thread: up to N contentsOfDirectory calls per render pass in sidebarScriptFileEntries, and a blocking atomic JSON write in setState. Both are called from user-facing, high-frequency paths — render body and tap actions. The native fallback means the app won't crash, but sidebar jank or hangs on slow volumes are a real consequence. Sources/ContentView.swift (sidebarScriptFileEntries and the render-sidebar context-building loop) and Sources/SidebarScriptStore.swift (setState blocking write) Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmux App (@MainActor)
participant Store as SidebarScriptStore
participant Script as SidebarScript
participant Eval as Evaluator
participant View as VerticalTabsSidebar / TabItemView
App->>Store: "init() [on @MainActor]"
Store->>Store: loadState(from:) — blocking Data(contentsOf:)
Store->>Store: reload() — blocking String(contentsOf:)
Store->>Script: SidebarScript(source:) — parse + eval top-level
Script->>Eval: eval() x N top-level forms
Script->>Store: baseEnv.freeze()
Store-->>App: script / version published
View->>Store: "@ObservedObject sidebarScriptStore"
Note over View: SwiftUI body evaluation
alt render-sidebar active
View->>View: makeSidebarScriptSidebarContext()
loop per workspace (up to 1000)
View->>View: sidebarScriptFileEntries() — blocking contentsOfDirectory on main thread
end
View->>Script: renderSidebar(context)
Script->>Eval: apply(render-sidebar, [context])
Script-->>View: RenderNode
View->>View: RenderNodeView(node:)
else render-row active
View->>Script: render(context) per visible row
Script->>Eval: apply(render-row, [context])
Script-->>View: RenderNode
end
View->>Store: setState(key:value:) [on tap action]
Store->>Store: state mutation (sync)
Store->>Store: createDirectory + encode + write(atomic:) — blocking FS on main thread
Reviews (9): Last reviewed commit: "Persist Finder sidebar Lisp state" | Re-trigger Greptile |
Bare tokens (tail/head/middle, vertical/horizontal/diagonal) used by the default and example scripts were unbound. Also lower the evaluator depth guard from 512 to 64 so a runaway script trips the bound instead of overflowing a small worker-thread Swift stack (the 512 limit crashed the test process with SIGBUS before throwing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return | ||
| } | ||
| do { | ||
| script = try SidebarScript(source: source) | ||
| // A within-run identity for the compiled script. `hashValue` is only | ||
| // compared within this process, where it is stable. | ||
| version = source.hashValue | ||
| Self.logger.info("Loaded custom sidebar.lisp (\(source.count) chars).") | ||
| } catch { | ||
| script = nil | ||
| version = 0 | ||
| Self.logger.error("sidebar.lisp failed to compile: \(String(describing: error), privacy: .public)") | ||
| } | ||
| } |
There was a problem hiding this comment.
@MainActor
SidebarScriptStore is @MainActor-isolated and is instantiated as a SwiftUI @State initializer on the main thread. String(contentsOf: url, encoding: .utf8) is a synchronous blocking filesystem call — it stalls if ~/.config/cmux/ is on a network volume, slow NFS mount, or locked by another process. More importantly, SidebarScript(source:) immediately follows: it runs the full reader, tokenizer, parser, and top-level evaluator (including all def forms in the script) synchronously on the main thread. For the bundled DefaultSidebar.lisp alone that's 121 lines of reader + eval work.
The fix is to move loading and compilation to a Task / background context and publish the result back to @MainActor once ready, keeping script and version as @Observable properties that start nil and update asynchronously.
| public final class SidebarScript { | ||
| private let baseEnv: LispEnvironment | ||
|
|
||
| /// The entry-point function name a script must define. | ||
| public static let entryPoint = "render-row" | ||
|
|
||
| public init(source: String) throws { | ||
| let forms = try Reader().read(source) | ||
| baseEnv = LispEnvironment() | ||
| Builtins.install(into: baseEnv) | ||
| Bridge.install(into: baseEnv) | ||
| let evaluator = Evaluator() | ||
| for form in forms { | ||
| _ = try evaluator.eval(form, in: baseEnv) | ||
| } | ||
| guard baseEnv.lookup(Self.entryPoint) != nil else { | ||
| throw LispError.eval(String( | ||
| localized: "sidebarScript.error.missingEntry", | ||
| defaultValue: "The script must define a 'render-row' function.", | ||
| bundle: .module)) | ||
| } | ||
| } | ||
|
|
||
| /// Renders one row to a pure node tree. Throws a localized `LispError` if the | ||
| /// script faults; the caller falls back to native rendering. | ||
| public func render(_ context: SidebarScriptContext) throws -> RenderNode { | ||
| guard let fn = baseEnv.lookup(Self.entryPoint) else { | ||
| throw LispError.eval(String( | ||
| localized: "sidebarScript.error.missingEntry", | ||
| defaultValue: "The script must define a 'render-row' function.", | ||
| bundle: .module)) | ||
| } | ||
| let evaluator = Evaluator() | ||
| let result = try evaluator.apply(fn, [context.lispValue]) | ||
| guard case .node(let node) = result else { | ||
| throw LispError.eval(String( | ||
| localized: "sidebarScript.error.entryReturn", | ||
| defaultValue: "'render-row' must return a view, but returned a \(result.typeName).", | ||
| bundle: .module)) | ||
| } | ||
| return node | ||
| } |
There was a problem hiding this comment.
set! on top-level globals contaminates baseEnv across row renders
SidebarScript.render creates a fresh Evaluator per call, but the closure captured by apply uses closureEnv which is baseEnv. A script that calls (set! counter (+ counter 1)) inside render-row — where counter is a top-level def — will walk up the scope chain via LispEnvironment.set, find the binding in baseEnv, and mutate it in place. Because the same baseEnv instance is reused for every render call, row N sees the accumulated mutations from rows 0…N-1 within the same frame, making renders non-deterministic and order-dependent.
One fix: have render create a child of baseEnv and pass that as the closure's execution scope so mutations are discarded after each row.
| } | ||
| ) | ||
| } | ||
|
|
||
| private func handleSidebarScriptAction(_ action: RNAction) { | ||
| switch action.kind { | ||
| case "open-url": |
There was a problem hiding this comment.
isDraft is always false in the PullRequest context projection
SidebarScriptContext.PullRequest exposes isDraft to scripts as the "draft" key in lispValue, but every PullRequest is constructed here with isDraft: false hardcoded. Scripts can never observe a PR as a draft regardless of actual state. Either plumb the real value through or remove the field from the public API to avoid misleading script authors.
| case "let", "let*": | ||
| return try evalLet(args, in: env) |
There was a problem hiding this comment.
let and let* are identical — both use sequential scoping
evalLet evaluates each binding value inside scope (the child env), so later bindings can see earlier ones. This is let* semantics. A user who relies on let's parallel-evaluation guarantee (all RHS evaluated in the outer env before any names are visible) will get surprising results. Consider either implementing true parallel let semantics, or collapsing both names to one form and documenting the let* behaviour explicitly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e5d42ab25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| branch: snapshot.compactGitBranchSummaryText, | ||
| directory: snapshot.compactDirectoryCandidates.first, |
There was a problem hiding this comment.
When the user has the vertical branch/directory sidebar layout enabled, makeWorkspaceSnapshot() deliberately leaves compactGitBranchSummaryText nil and compactDirectoryCandidates empty and puts the visible data in branchDirectoryLines; this context builder only reads the compact fields, so every sidebar script (including the bundled default) loses branch/path data in that configuration even though branch-directory details are enabled. Please populate the script context from the vertical-layout snapshot as a fallback or expose those lines separately.
Useful? React with 👍 / 👎.
| case "line-limit": | ||
| return AnyView(view.lineLimit(Int(m.first?.number ?? 1))) |
There was a problem hiding this comment.
When a custom script uses the exposed infinity constant (for example (text "x" :line-limit infinity)) or produces NaN, the script compiles successfully and this renderer later executes Int(Double.infinity)/Int(Double.nan), which is a Swift fatal trap rather than a thrown render error, so the native-row fallback cannot catch it and the app crashes. Please reject or clamp non-finite values before converting line limits to Int.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 28 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift:87">
P2: Double equality here breaks on NaN. Same value can compare unequal and force false change detection. Handle NaN explicitly.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:14993">
P2: Per-row render failure logs spam fast. One bad script can flood logs on every row re-render. Gate or throttle this log.</violation>
<violation number="2" location="Sources/ContentView.swift:15013">
P2: `isDraft` is hardcoded to `false` here, so the `:draft` field in the workspace record passed to scripts is never true. The bundled `DefaultSidebar.lisp` checks `(get pr :draft)` in `pr-color` to render draft PRs gray, but that branch can never trigger—draft PRs always appear green. Plumb the actual draft status from the PR row data.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift:159">
P1: `Int(m.first?.number ?? 1)` will crash with a fatal trap if the number is `.infinity` or `.nan` (e.g. a script using `(text "x" :line-limit infinity)`). The bridge exposes `infinity` as a constant, so this is easily reachable. Clamp or reject non-finite values before the `Int` conversion to prevent an unrecoverable crash that bypasses the native-row fallback.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift:273">
P2: Bad hex color becomes transparent. Script typo can hide row content and mask the fault. Use a visible fallback or fail color validation earlier.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:22">
P1: Step budget only ticks in `eval`. Builtin loops can run huge work without spending steps, so script can still block UI. Add budget checks in function application and builtin iteration paths.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:162">
P2: `let` evaluates each binding value in `scope` (the child env), giving it `let*` semantics where later bindings can reference earlier ones. Standard `let` should evaluate all RHS expressions in the outer `env` before defining any names. Either fix by evaluating in `env` instead of `scope`, or collapse `let`/`let*` into one form and document the sequential behavior.</violation>
<violation number="3" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:221">
P2: Rest-param parse ignores extra tokens after `&`. Bad signatures compile silently and drop parameters. Reject anything after the rest name.</violation>
</file>
<file name="Sources/SidebarScriptStore.swift">
<violation number="1" location="Sources/SidebarScriptStore.swift:30">
P1: Error text logged as public. Can leak user script data in logs. Log private/redacted error details instead.</violation>
<violation number="2" location="Sources/SidebarScriptStore.swift:40">
P2: MainActor init does sync file read + compile. Can stall sidebar/UI startup. Move load/compile off main actor, then publish result back on MainActor.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift:95">
P2: Unknown string escape drops backslash. Parser changes user text silently. Keep backslash or throw read error.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift:167">
P3: Dangling quote loses line number. Error points less clearly to bad script. Guard missing quoted form and throw with quote line.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift:47">
P2: Arity check too loose for rgb/rgba. Extra args get ignored silently. Enforce exact arg count.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift:221">
P2: Repeated modifier key applied multiple times. Last value picked, but modifier still stacks. Skip duplicate keys before apply.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:170">
P2: `record` drops a dangling key silently. Bad input should throw, not partially apply.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swift:60">
P2: Resource load failure gets swallowed. Empty fallback hides real default-script error. Return/throw explicit load failure.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| /// Logs a per-row render failure. Called from the sidebar row when a script | ||
| /// faults so the row can fall back to native rendering. | ||
| static func logRenderFailure(_ error: Error) { | ||
| logger.error("sidebar.lisp render failed: \(String(describing: error), privacy: .public)") |
There was a problem hiding this comment.
P1: Error text logged as public. Can leak user script data in logs. Log private/redacted error details instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/SidebarScriptStore.swift, line 30:
<comment>Error text logged as public. Can leak user script data in logs. Log private/redacted error details instead.</comment>
<file context>
@@ -0,0 +1,57 @@
+ /// Logs a per-row render failure. Called from the sidebar row when a script
+ /// faults so the row can fall back to native rendering.
+ static func logRenderFailure(_ error: Error) {
+ logger.error("sidebar.lisp render failed: \(String(describing: error), privacy: .public)")
+ }
+
</file context>
|
|
||
| public func eval(_ form: LispValue, in env: LispEnvironment) throws -> LispValue { | ||
| steps += 1 | ||
| if steps > stepLimit { throw LispError.stepLimit } |
There was a problem hiding this comment.
P1: Step budget only ticks in eval. Builtin loops can run huge work without spending steps, so script can still block UI. Add budget checks in function application and builtin iteration paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift, line 22:
<comment>Step budget only ticks in `eval`. Builtin loops can run huge work without spending steps, so script can still block UI. Add budget checks in function application and builtin iteration paths.</comment>
<file context>
@@ -0,0 +1,274 @@
+
+ public func eval(_ form: LispValue, in env: LispEnvironment) throws -> LispValue {
+ steps += 1
+ if steps > stepLimit { throw LispError.stepLimit }
+
+ switch form {
</file context>
| case "rotation": | ||
| return AnyView(view.rotationEffect(.degrees(m.first?.number ?? 0))) | ||
| case "line-limit": | ||
| return AnyView(view.lineLimit(Int(m.first?.number ?? 1))) |
There was a problem hiding this comment.
P1: Int(m.first?.number ?? 1) will crash with a fatal trap if the number is .infinity or .nan (e.g. a script using (text "x" :line-limit infinity)). The bridge exposes infinity as a constant, so this is easily reachable. Clamp or reject non-finite values before the Int conversion to prevent an unrecoverable crash that bypasses the native-row fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift, line 159:
<comment>`Int(m.first?.number ?? 1)` will crash with a fatal trap if the number is `.infinity` or `.nan` (e.g. a script using `(text "x" :line-limit infinity)`). The bridge exposes `infinity` as a constant, so this is easily reachable. Clamp or reject non-finite values before the `Int` conversion to prevent an unrecoverable crash that bypasses the native-row fallback.</comment>
<file context>
@@ -0,0 +1,483 @@
+ case "rotation":
+ return AnyView(view.rotationEffect(.degrees(m.first?.number ?? 0)))
+ case "line-limit":
+ return AnyView(view.lineLimit(Int(m.first?.number ?? 1)))
+ case "kerning":
+ return AnyView(view.kerning(CGFloat(m.first?.number ?? 0)))
</file context>
| } | ||
|
|
||
| init(url: URL = SidebarScriptStore.scriptURL) { | ||
| guard let source = try? String(contentsOf: url, encoding: .utf8) else { |
There was a problem hiding this comment.
P2: MainActor init does sync file read + compile. Can stall sidebar/UI startup. Move load/compile off main actor, then publish result back on MainActor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/SidebarScriptStore.swift, line 40:
<comment>MainActor init does sync file read + compile. Can stall sidebar/UI startup. Move load/compile off main actor, then publish result back on MainActor.</comment>
<file context>
@@ -0,0 +1,57 @@
+ }
+
+ init(url: URL = SidebarScriptStore.scriptURL) {
+ guard let source = try? String(contentsOf: url, encoding: .utf8) else {
+ script = nil
+ version = 0
</file context>
| case (.null, .null): return true | ||
| case let (.bool(a), .bool(b)): return a == b | ||
| case let (.int(a), .int(b)): return a == b | ||
| case let (.double(a), .double(b)): return a == b |
There was a problem hiding this comment.
P2: Double equality here breaks on NaN. Same value can compare unequal and force false change detection. Handle NaN explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift, line 87:
<comment>Double equality here breaks on NaN. Same value can compare unequal and force false change detection. Handle NaN explicitly.</comment>
<file context>
@@ -0,0 +1,100 @@
+ case (.null, .null): return true
+ case let (.bool(a), .bool(b)): return a == b
+ case let (.int(a), .int(b)): return a == b
+ case let (.double(a), .double(b)): return a == b
+ case let (.int(a), .double(b)), let (.double(b), .int(a)): return Double(a) == b
+ case let (.string(a), .string(b)): return a == b
</file context>
|
|
||
| private func color(_ c: RNColor) -> Color { | ||
| switch c { | ||
| case .hex(let s): return Color(hex: s) ?? .clear |
There was a problem hiding this comment.
P2: Bad hex color becomes transparent. Script typo can hide row content and mask the fault. Use a visible fallback or fail color validation earlier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift, line 273:
<comment>Bad hex color becomes transparent. Script typo can hide row content and mask the fault. Use a visible fallback or fail color validation earlier.</comment>
<file context>
@@ -0,0 +1,483 @@
+
+ private func color(_ c: RNColor) -> Color {
+ switch c {
+ case .hex(let s): return Color(hex: s) ?? .clear
+ case .rgba(let r, let g, let b, let a):
+ let (rr, gg, bb) = normalizeRGB(r, g, b)
</file context>
| throw LispError.eval(String(localized: "sidebarScript.error.restParam", | ||
| defaultValue: "'&' must be followed by one rest parameter.", bundle: .module)) | ||
| } | ||
| rest = r |
There was a problem hiding this comment.
P2: Rest-param parse ignores extra tokens after &. Bad signatures compile silently and drop parameters. Reject anything after the rest name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift, line 221:
<comment>Rest-param parse ignores extra tokens after `&`. Bad signatures compile silently and drop parameters. Reject anything after the rest name.</comment>
<file context>
@@ -0,0 +1,274 @@
+ throw LispError.eval(String(localized: "sidebarScript.error.restParam",
+ defaultValue: "'&' must be followed by one rest parameter.", bundle: .module))
+ }
+ rest = r
+ break
+ }
</file context>
| defaultValue: "Each 'let' binding must be (name value).", bundle: .module)) | ||
| } | ||
| // Sequential binding (let*): later bindings see earlier ones. | ||
| scope.define(name, try eval(pair[1], in: scope)) |
There was a problem hiding this comment.
P2: let evaluates each binding value in scope (the child env), giving it let* semantics where later bindings can reference earlier ones. Standard let should evaluate all RHS expressions in the outer env before defining any names. Either fix by evaluating in env instead of scope, or collapse let/let* into one form and document the sequential behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift, line 162:
<comment>`let` evaluates each binding value in `scope` (the child env), giving it `let*` semantics where later bindings can reference earlier ones. Standard `let` should evaluate all RHS expressions in the outer `env` before defining any names. Either fix by evaluating in `env` instead of `scope`, or collapse `let`/`let*` into one form and document the sequential behavior.</comment>
<file context>
@@ -0,0 +1,274 @@
+ defaultValue: "Each 'let' binding must be (name value).", bundle: .module))
+ }
+ // Sequential binding (let*): later bindings see earlier ones.
+ scope.define(name, try eval(pair[1], in: scope))
+ }
+ return try evalBody(Array(args.dropFirst()), in: scope)
</file context>
| scope.define(name, try eval(pair[1], in: scope)) | |
| scope.define(name, try eval(pair[1], in: env)) |
| state: $0.status.rawValue, | ||
| url: $0.url.absoluteString, | ||
| title: $0.label, | ||
| isDraft: false, |
There was a problem hiding this comment.
P2: isDraft is hardcoded to false here, so the :draft field in the workspace record passed to scripts is never true. The bundled DefaultSidebar.lisp checks (get pr :draft) in pr-color to render draft PRs gray, but that branch can never trigger—draft PRs always appear green. Plumb the actual draft status from the PR row data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 15013:
<comment>`isDraft` is hardcoded to `false` here, so the `:draft` field in the workspace record passed to scripts is never true. The bundled `DefaultSidebar.lisp` checks `(get pr :draft)` in `pr-color` to render draft PRs gray, but that branch can never trigger—draft PRs always appear green. Plumb the actual draft status from the PR row data.</comment>
<file context>
@@ -14965,6 +14977,75 @@ struct TabItemView: View, Equatable {
+ state: $0.status.rawValue,
+ url: $0.url.absoluteString,
+ title: $0.label,
+ isDraft: false,
+ isStale: $0.isStale
+ )
</file context>
| isDraft: false, | |
| isDraft: $0.isDraft, |
| defaultValue: "Unexpected ')'.", bundle: .module), line: l) | ||
| case .quote: | ||
| index += 1 | ||
| let quoted = try parseForm() |
There was a problem hiding this comment.
P3: Dangling quote loses line number. Error points less clearly to bad script. Guard missing quoted form and throw with quote line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift, line 167:
<comment>Dangling quote loses line number. Error points less clearly to bad script. Guard missing quoted form and throw with quote line.</comment>
<file context>
@@ -0,0 +1,209 @@
+ defaultValue: "Unexpected ')'.", bundle: .module), line: l)
+ case .quote:
+ index += 1
+ let quoted = try parseForm()
+ return .list([.symbol("quote"), quoted])
+ case let .string(s, _):
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🤖 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/CmuxSidebarScript/Package.swift`:
- Around line 22-34: Remove the explicit .swiftLanguageMode(.v5) overrides in
the Package.swift target definitions for CmuxSidebarScript and
CmuxSidebarScriptTests (search for the swiftSettings arrays in those target
blocks) so the package uses the package-level swift-tools-version (Swift 6);
alternatively set them to .swiftLanguageMode(.v6) if you want to be explicit,
then compile and address any Swift 6 concurrency diagnostics introduced in
functions/types referenced by those targets (resolve
Sendable/Sendable-conformance, actor isolation, or async usage in the code paths
that failed).
In `@Packages/CmuxSidebarScript/README.md`:
- Around line 11-13: Update the Markdown code fence that contains the pipeline
diagram so it includes a language tag (e.g., change "```" to "```text") to
satisfy MD040; locate the fence containing the line "source ──Reader──▶
[LispValue] ──Evaluator + Bridge──▶ RenderNode ──RenderNodeView──▶ SwiftUI" in
the README and add the language tag (or convert to a mermaid block if you
prefer) so the fence starts with ```text.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift`:
- Around line 217-245: The finish(_ base:viewName:options:) function currently
iterates every option occurrence and still emits modifiers for repeated keys;
change it to deduplicate so only the last occurrence of each repeatable key is
applied. Update finish to compute the last-value map for options (ignoring keys
in frameKeys and borderKeys which are already grouped) or keep a seen set so
when iterating you skip earlier repeats and only call
applyModifier(node,key,value,viewName) for the last value; ensure frame handling
(frameKeys) and border handling (borderKeys) still build a single RenderModifier
each as before and reference the same symbol names (finish, options, frameKeys,
borderKeys, applyModifier, RenderModifier) so duplicate modifiers like
padding/background/opacity/help are emitted only once with the last provided
value.
- Around line 69-75: installActionConstructors currently serializes a missing
argument via Builtins.display(args.first ?? .null) for the builtin("open-url")
and builtin("copy-text") actions, producing the literal "nil"; instead detect
when args.first is missing or equals .null and reject the action rather than
constructing .style(.action(...)). Update the builtin handlers in
installActionConstructors (for "open-url" and "copy-text") to check args.first
and, if absent/.null, return an error/rejection value or throw so the
caller/compiler can trigger the existing fallback, otherwise build the RNAction
with the validated payload (use Builtins.display only for present/non-null
values).
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift`:
- Around line 167-177: The record and assoc builtins currently iterate with
while i + 1 < args.count which silently ignores a trailing key; update their
argument validation to detect odd-length key/value lists and throw an eval error
instead of dropping the final key: either check args.count % 2 == 0 up front or
after the loop verify i == args.count and throw a LispError.eval with an
appropriate localized message (use the same localization system as the existing
error) when a key is missing; apply this change to the def("record")
implementation and the def("assoc") implementation to ensure malformed calls
like (record :a 1 :b) or (assoc rec :a 1 :b) raise an error.
- Around line 244-258: The rangeBuiltin currently emits every value as
.int(Int(x)) which truncates fractional values; update rangeBuiltin to preserve
numeric precision by emitting .number(x) when x is not an integer and only use
.int(Int(x)) for whole numbers (e.g., test whether x.rounded() == x), or
alternatively validate inputs up front and throw a LispError if non-integral
start/end/step are provided; adjust the emission logic where
out.append(.int(Int(x))) occurs to append .number(x) when needed and keep
existing behavior for exact integers so downstream consumers of LispValue
receive correct numeric types.
- Around line 35-36: The current def("not=") uses allAdjacent and only compares
neighbors, so change its closure to verify pairwise distinctness across all
arguments: replace def("not=") { args, _ in .bool(!allAdjacent(args) { $0 == $1
}) } with a closure that iterates i from 0..<args.count and j from
i+1..<args.count and returns .bool(false) if any args[i] == args[j], otherwise
.bool(true); reference def("not="), allAdjacent and def("=") to locate the
related equality helpers and ensure you use the Value equality operator already
used elsewhere.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Errors.swift`:
- Around line 7-34: Add Swift-DocC triple-slash documentation for the public
API: document the type LispError, its nested enum LispError.Stage, the public
properties stage, message, line, and the public init(stage:message:line:) with a
one-sentence summary ending with a period, an optional blank line then a short
discussion, and include `@Parameters` for stage/message/line and `@Returns/`@Throws
where appropriate plus a short fenced code example demonstrating construction
and use; ensure the Stage cases (read, eval) are described, the line property
notes 1-based indexing and optionality, and place these /// comments immediately
above the LispError declaration, the Stage enum, each public property (if
desired), and the initializer to satisfy symbol-level DocC requirements.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift`:
- Around line 20-23: The depthLimit check must run at every evaluation frame, so
move or duplicate the depth accounting into the shared evaluator entry
(eval(_:in:)) instead of only inside applyClosure; specifically, increment a
recursion-depth counter (or use the existing steps/depth variables) at the start
of public func eval(_ form: LispValue, in env: LispEnvironment) and throw
LispError.depthLimit when it exceeds depthLimit, then ensure other callsites
(e.g. applyClosure) no longer rely on being the sole enforcer; keep stepLimit
logic separate and preserved.
- Around line 113-115: The bug is that both "let" and "let*" call the same
helper so bindings are evaluated sequentially (like let*); change the
implementation so "let" evaluates all binding expressions in the outer
environment without seeing earlier bindings while "let*" evaluates them
sequentially seeing prior bindings. Locate the evaluator switch that dispatches
to evalLet and either split evalLet into two helpers (e.g., evalLet and
evalLetStar) or add a flag to evalLet (e.g., sequential: Bool) to control
whether the temporary scope is updated between bindings; ensure calls from the
evaluator use the correct helper/flag for "let" vs "let*" and update any tests
or callers that assumed the previous behavior.
- Around line 207-227: parseParams currently stops parsing when it sees "&" and
sets rest but allows any additional forms to be silently ignored; change
parseParams(_:) so that when encountering the "&" symbol it requires exactly one
following symbol (the rest name) and then verifies there are no further forms
after that, otherwise throw a LispError.eval with the same
"sidebarScript.error.restParam" message; update the logic around the guard that
reads the rest (using forms[i + 1]) to check i + 2 == forms.count (or
equivalent) before accepting the rest and returning params and rest.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift`:
- Around line 11-19: Add Swift-DocC triple-slash documentation for the public
symbols Reader.init() and Reader.read(_:): for each symbol include a
one-sentence summary ending with a period, an optional blank line and short
discussion paragraph, and the required callouts — for read(_:), add `@Parameter`
source describing the input string, `@Returns` describing the returned
[LispValue], and `@Throws` describing possible thrown errors — and include an
example usage in a fenced code block; place the doc comments immediately above
the public init() and public func read(_:) declarations so Xcode/DocC and the
package linter will pick them up.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNode.swift`:
- Around line 20-24: Add Swift-DocC triple-slash documentation for each exposed
public symbol: document RenderNode.init(kind:content:children:modifiers:), the
static RenderNode.empty, RenderModifier.name, RenderModifier.init(...), and
RenderModifier.first; place a concise description above each declaration,
include parameter descriptions for init parameters, explain return/behavior for
empty and first, and note any important invariants or usage examples where
helpful—ensure the comments use /// syntax immediately above the declarations so
the public API is fully documented.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift`:
- Around line 6-18: RenderNodeView exposes public API members that lack
Swift-DocC documentation: add triple-slash (///) comments for the public
property `node`, the public initializer `init(node:onAction:)`, and the public
computed property `body` in the RenderNodeView type so they meet package
guidelines; each doc comment should briefly describe the purpose, parameters
(for the initializer), and behavior/return value (for `body`) and mention the
optional `onAction` handler and its RNAction parameter where relevant.
- Around line 132-134: The padding branch only handles the .edges case so scalar
paddings like .padding(8) are no-ops; update the case "padding" in
RenderNodeView to also detect a numeric scalar modifier from m.first (the same
place that currently checks if case .edges(let e)? = m.first) and when present
convert it to a CGFloat and return AnyView(view.padding(CGFloat(value))).
Preserve the existing edges handling (if case .edges(let e)? = m.first { return
AnyView(view.padding(insets(e))) }) and only fall back to the unchanged view
when no valid modifier is present.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderValues.swift`:
- Around line 21-124: Add Swift-DocC triple-slash documentation comments for
each public API introduced here: the RNValue accessors number and string, the
RNColor enum (and its cases hex, rgba, named, semantic), RNFont (and its
properties and init), RNGradient and Direction, RNAlignment, RNEdges (including
uniform(_:)), RNShadow, and RNAction (and its init). For each symbol add a brief
summary line and where appropriate document parameters/return values (e.g., ///
Returns the numeric/string value if this RNValue is a .number/.string), describe
fields (e.g., /// Size in points) and initializer parameters so the public
surface is fully documented with triple-slash Swift-DocC comments.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swift`:
- Line 19: Document the public initializer by adding a Swift-DocC triple-slash
comment immediately above public init(source: String) that describes what
SidebarScript represents, what the source parameter is (expected
format/contents), and any conditions under which the initializer throws (include
the specific error types or cases thrown). Mention ownership/usage notes if
relevant (e.g., whether source is copied or retained) and include a short
one-line example or note about typical usage. Ensure the comment is placed
directly on the public init(source:) symbol so it appears in generated docs and
satisfies the package documentation guideline.
- Around line 28-33: The initializer currently only checks that Self.entryPoint
(the "render-row" symbol) exists but not that it's a callable function, so
update init(source:) to retrieve the binding from
baseEnv.lookup(Self.entryPoint) and verify its type is a function/closure before
returning; if the binding is not callable, throw the same LispError.eval with
the localized "sidebarScript.error.missingEntry" message (or a new message
indicating a non-callable entry) to fail fast. Locate the checks around
baseEnv.lookup(Self.entryPoint) in SidebarScript.init(source:) and the analogous
block at the later check (lines ~39-46) and add a runtime/type guard that
ensures the found value is a callable (function/closure) according to your Lisp
value representation, rejecting non-function bindings with an error. Ensure you
reference the entryPoint symbol and preserve existing localization and error
type (LispError.eval) when throwing.
- Around line 57-67: defaultSource() currently swallows a missing bundled file
by returning an empty string which causes makeDefault()/SidebarScript
initializer to fail with an unrelated runtime error; change defaultSource() to
signal failure (either make it throwing or return an optional) when
Bundle.module.url(...) or String(contentsOf:) fails, and update makeDefault() to
call the new defaultSource() and throw a clear, descriptive error (e.g.,
"missing DefaultSidebar.lisp in bundle" or propagate the underlying error) so
packaging/integration problems are surfaced immediately; reference functions
defaultSource(), makeDefault(), and the SidebarScript initializer when applying
the change.
In
`@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptContext.swift`:
- Around line 10-92: Add Swift-DocC triple-slash comments for each public symbol
in this file: document the PullRequest type and each of its public stored
properties (number, state, url, title, isDraft, isStale) and its initializer;
document the StatusEntry type and each property (label, value, colorHex) and its
initializer; and document the SidebarScriptContext public stored properties
(title, detail, branch, directory, directories, pullRequests, ports,
unreadCount, isPinned, isActive, isSelected, colorHex, isDarkMode,
latestMessage, progress, remoteTarget, statusEntries) plus the public init(...)
signature — for each symbol add a concise triple-slash description above the
declaration describing purpose and semantics per DocC guidelines.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/StyleValue.swift`:
- Around line 7-13: Add Swift-DocC triple-slash comments to each public
StyleValue enum case (color, font, gradient, alignment, edges, shadow, action)
describing the render-layer value produced by that constructor; update the
declarations in the StyleValue enum so each case has a concise /// comment that
documents the produced render-layer type and any important semantics or units
(e.g., color -> RGBA render color, edges -> padding/margin edges), following
package coding guidelines for public symbols.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift`:
- Around line 9-99: Add full DocC comments for the public API surface: add
triple-slash documentation for the enum LispValue's public helpers isTruthy,
typeName, asDouble, and the static constructor number(_:). For each symbol
provide a one-sentence summary ending with a period, an optional blank line and
short discussion paragraph, appropriate `@returns/`@parameter callouts (as
relevant: isTruthy -> `@returns` Bool, typeName -> `@returns` String, asDouble ->
`@returns` Double?, number(_:) -> `@parameter` d: Double and `@returns` LispValue),
and a short fenced code example showing typical usage; attach these comments
immediately above the declarations of isTruthy, typeName, asDouble, and
number(_:) so DocC picks them up.
In
`@Packages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/EvaluatorTests.swift`:
- Line 83: Replace the force-try in the test setup so failures produce test
failures instead of aborting: change the `let forms = try!
Reader().read("(reduce + 0 (range 100))")` usage of Reader().read to a
non-crashing pattern—either mark the test method as `throws` and use `let forms
= try Reader().read(...)`, or wrap the call with `XCTAssertNoThrow` (and capture
the result) so errors from `Reader().read` are reported by the test runner
rather than crashing the process.
In
`@Packages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/SidebarScriptTests.swift`:
- Around line 35-36: Replace the loose image existence assertion with one that
specifically targets the branch symbol: instead of asserting
node.firstNode(kind: "image") != nil, locate the image node that represents the
branch (e.g. the image whose symbol/name is "arrow.triangle.branch") using
node.firstNode(...) or a branch-specific matcher and assert that that node
exists; reference the test helper firstNode(kind: ...) and the
branch-line/branch symbol name to ensure the branch row is actually present.
- Around line 51-53: The current assertion uses node.containsText("#") which
only matches an exact "#" and misses PR labels like "`#5108`"; update the test to
assert no PR rows by either checking that node.nodes(kind: "pr-row").isEmpty or
by scanning text nodes for a leading "#" (e.g. ensure no text node hasPrefix
"#") instead of the exact-match containsText("#"), so that any leaked PR entries
are detected; modify the assertion around node.nodes(kind: "progress-view") /
node.containsText("#") accordingly.
In `@Sources/ContentView.swift`:
- Around line 14982-15030: renderedSidebarScriptNode currently rebuilds
SidebarScriptContext and invokes the Lisp renderer on every TabItemView.body
pass (triggered by ForEach row invalidations), causing heavy repeated
interpreter work on the hot UI path; fix by moving rendering off the body hot
path — compute and cache the rendered RenderNode keyed by a stable input (for
example a hash of SidebarScriptContext fields or a versioned snapshot id)
upstream or in a view-model, then change renderedSidebarScriptNode/TabItemView
to look up the cached RenderNode instead of recomputing; ensure the cache is
invalidated only when relevant inputs (directory, title, pullRequests, ports,
unreadCount, isPinned, isActive, latestMessage, progress, statusEntries, etc.)
change so the Lisp renderer is not called during ordinary hover/selection/drag
updates.
- Around line 15033-15046: The scripted subtree is currently wired with onAction
(via RenderNodeView) which lets script-level button/actions consume primary
pointer events and prevent the row's native select/open/drag behavior; remove or
stop passing onAction into RenderNodeView for sidebar rows (the places that call
RenderNodeView around handleSidebarScriptAction and the sibling block at
15079-15083) so script actions are not invoked on primary taps — instead provide
actions via a non-primary channel (context menu or separate action prop) or only
wire action handlers for non-primary events; update code that constructs
RenderNodeView to omit onAction for sidebar rows and keep
handleSidebarScriptAction as the dedicated handler for explicit
non-primary/script-triggered invocations.
In `@Sources/SidebarScriptStore.swift`:
- Around line 40-43: Replace the silent try? in SidebarScriptStore.init(url:)
with a do-catch: attempt to read String(contentsOf: url, encoding: .utf8) and on
success assign script/version as before; if reading fails first check
FileManager.default.fileExists(atPath: url.path) and if the file does not exist
keep script = nil and version = 0 silently, otherwise log the actual error
(e.g., via NSLog/os_log or the project's logger) so permission/decoding errors
are reported; update the error branch to set script/version the same as the
missing-file branch while adding the logged error.
- Around line 39-50: The init of SidebarScriptStore performs synchronous disk
I/O and compilation on the `@MainActor`; change it so init immediately sets script
= nil and version = 0 and then spawns a background task (e.g. Task.detached or a
global DispatchQueue) to read SidebarScriptStore.scriptURL, create the
SidebarScript (which calls Reader().read and evaluator.eval), compute the
version (source.hashValue), and then publish the results back on the main actor
(via await MainActor.run { self.script = ...; self.version = ... }) so
loading/compilation no longer blocks VerticalTabsSidebar construction; keep all
assignments to script/version on the main actor and handle read/compile failures
by leaving script = nil/version = 0.
🪄 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: 3ec7fd72-e57b-4ddb-85a7-f10835e1aee3
📒 Files selected for processing (28)
Packages/CmuxSidebarScript/Package.swiftPackages/CmuxSidebarScript/README.mdPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Coercion.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Errors.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispFunction.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispMap.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNode.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderValues.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Resources/DefaultSidebar.lispPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptContext.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/StyleValue.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/BridgeTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/BuiltinTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/EvaluatorTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/ReaderTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/SidebarScriptTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/TestSupport.swiftSources/ContentView.swiftSources/SidebarScriptStore.swiftcmux.xcodeproj/project.pbxproj
| swiftSettings: [ | ||
| // The engine runs synchronously on the main thread behind a single | ||
| // entry point; language mode 5 keeps the small interpreter free of | ||
| // Sendable ceremony that buys nothing here. | ||
| .swiftLanguageMode(.v5), | ||
| ] | ||
| ), | ||
| .testTarget( | ||
| name: "CmuxSidebarScriptTests", | ||
| dependencies: ["CmuxSidebarScript"], | ||
| swiftSettings: [ | ||
| .swiftLanguageMode(.v5), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all explicit language-mode overrides in package manifests.
fd -a "Package.swift" | xargs -I{} sh -c 'echo "## {}"; nl -ba "{}" | rg -n "swiftLanguageMode|swift-tools-version"'Repository: manaflow-ai/cmux
Length of output: 1586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the touched manifest first
echo "=== Packages/CmuxSidebarScript/Package.swift matches ==="
rg -n "swiftLanguageMode|swift-tools-version" Packages/CmuxSidebarScript/Package.swift || true
echo
echo "=== Packages/CmuxSidebarScript/Package.swift (lines 1-120) ==="
sed -n '1,120p' Packages/CmuxSidebarScript/Package.swift
# Then scan for any Swift language mode pins to assess scope
echo
echo "=== Repo-wide pins: swiftLanguageMode(.v5) in Package.swift files ==="
rg -n "swiftLanguageMode\\(\\.v5\\)" --glob "**/Package.swift" . || trueRepository: manaflow-ai/cmux
Length of output: 1525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la .github/review-bot-rules || true
fd -a "concurrency|Package|language mode|swiftLanguageMode|swift-tools-version" .github/review-bot-rules -t f --exec sh -c 'echo "---- $1"; sed -n "1,200p" "$1"' sh {}Repository: manaflow-ai/cmux
Length of output: 2963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "swiftLanguageMode|language mode|Swift 5|Swift 6|swift-tools-version" .github/review-bot-rules/*.md .github/review-bot-rules/README.md || true
# Also dump any README if present
[ -f .github/review-bot-rules/README.md ] && sed -n '1,200p' .github/review-bot-rules/README.md || trueRepository: manaflow-ai/cmux
Length of output: 1733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "tools-version|swiftLanguageMode" .github/review-bot-rules || trueRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== swift-tools-version values across all Package.swift ==="
rg -n "swift-tools-version" --glob "**/Package.swift" . || true
echo
echo "=== swiftLanguageMode pins across all Package.swift ==="
rg -n "swiftLanguageMode\\(\\.[^)]+\\)" --glob "**/Package.swift" . || trueRepository: manaflow-ai/cmux
Length of output: 1992
🌐 Web query:
SwiftPM PackageDescription swiftLanguageMode .v5 .v6 what does it do concurrency checking NonisolatedNonsendingByDefault
💡 Result:
In SwiftPM, PackageDescription’s swiftLanguageMode (API spelling “swiftLanguageModes”) controls which Swift language mode the compiler uses when building the package/targets (e.g.,.v5 vs.v6), which in practice turns on Swift 6-era rules for concurrency checking (stricter sendability/isolation diagnostics). [1][2][3] NonisolatedNonsendingByDefault is a separate concurrency-related upcoming feature flag: when enabled, the default for “nonisolated async” becomes nonisolated(nonsending), and it also affects how nonisolated async parameters/closures inherit isolation semantics. [4][5][6] So: - swiftLanguageMode.v5 /.v6: selects Swift 5 vs Swift 6 language-mode semantics (including stricter concurrency checking in.v6). [1][3] - NonisolatedNonsendingByDefault: refines how the compiler treats “nonisolated” defaults for async (and related params/closures) during Swift 6-era concurrency checking. [4][5][6]
Citations:
- 1: https://github.com/apple/swift-package-manager/blob/main/CHANGELOG.md
- 2: Explicitly set the Swift language mode on package init swiftlang/swift-package-manager#9263
- 3: https://www.donnywals.com/setting-the-swift-language-mode-for-an-spm-package/
- 4: https://forums.swift.org/t/questions-about-swift-6-concurrency/82045
- 5: https://www.donnywals.com/exploring-concurrency-changes-in-swift-6-2/
- 6: https://forums.swift.org/t/sendable-async-closures-and-nonisolated-nonsending-behavior/86951
Stop pinning CmuxSidebarScript to Swift 5 language mode; use Swift 6 in Package.swift
Packages/CmuxSidebarScript/Package.swift sets .swiftLanguageMode(.v5) for both the CmuxSidebarScript target and CmuxSidebarScriptTests (while swift-tools-version is already 6.0, and other packages use .swiftLanguageMode(.v6)). SwiftPM’s swiftLanguageMode selects compiler language/concurrency-checking semantics, so .v5 weakens Swift 6 concurrency diagnostics. Remove the .v5 overrides and fix any resulting Swift 6 concurrency errors.
🤖 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/CmuxSidebarScript/Package.swift` around lines 22 - 34, Remove the
explicit .swiftLanguageMode(.v5) overrides in the Package.swift target
definitions for CmuxSidebarScript and CmuxSidebarScriptTests (search for the
swiftSettings arrays in those target blocks) so the package uses the
package-level swift-tools-version (Swift 6); alternatively set them to
.swiftLanguageMode(.v6) if you want to be explicit, then compile and address any
Swift 6 concurrency diagnostics introduced in functions/types referenced by
those targets (resolve Sendable/Sendable-conformance, actor isolation, or async
usage in the code paths that failed).
| ``` | ||
| source ──Reader──▶ [LispValue] ──Evaluator + Bridge──▶ RenderNode ──RenderNodeView──▶ SwiftUI | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the pipeline code fence.
This currently trips markdown lint (MD040). Use text (or mermaid if you convert it) for the fence language.
Suggested fix
-```
+```text
source ──Reader──▶ [LispValue] ──Evaluator + Bridge──▶ RenderNode ──RenderNodeView──▶ SwiftUI</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/CmuxSidebarScript/README.md` around lines 11 - 13, Update the
Markdown code fence that contains the pipeline diagram so it includes a language
tag (e.g., change "```" to "```text") to satisfy MD040; locate the fence
containing the line "source ──Reader──▶ [LispValue] ──Evaluator + Bridge──▶
RenderNode ──RenderNodeView──▶ SwiftUI" in the README and add the language tag
(or convert to a mermaid block if you prefer) so the fence starts with ```text.
| private static func installActionConstructors(_ env: LispEnvironment) { | ||
| builtin("open-url", env) { args, _ in | ||
| .style(.action(RNAction(kind: "open-url", payload: ["url": Builtins.display(args.first ?? .null)]))) | ||
| } | ||
| builtin("copy-text", env) { args, _ in | ||
| .style(.action(RNAction(kind: "copy", payload: ["text": Builtins.display(args.first ?? .null)]))) | ||
| } |
There was a problem hiding this comment.
Reject missing action payloads instead of serializing nil.
Lines 71 and 74 turn a missing argument into the literal string "nil", so (open-url) yields an unusable URL payload and (copy-text) copies "nil" instead of failing the script. That bypasses the intended compile/render fallback for malformed actions.
Suggested fix
private static func installActionConstructors(_ env: LispEnvironment) {
builtin("open-url", env) { args, _ in
- .style(.action(RNAction(kind: "open-url", payload: ["url": Builtins.display(args.first ?? .null)])))
+ guard let first = args.first else {
+ throw LispError.arity("open-url", expected: "a URL", got: 0)
+ }
+ return .style(.action(
+ RNAction(kind: "open-url", payload: ["url": try Coercion.string(first, "open-url")])
+ ))
}
builtin("copy-text", env) { args, _ in
- .style(.action(RNAction(kind: "copy", payload: ["text": Builtins.display(args.first ?? .null)])))
+ guard let first = args.first else {
+ throw LispError.arity("copy-text", expected: "a value", got: 0)
+ }
+ return .style(.action(
+ RNAction(kind: "copy", payload: ["text": Builtins.display(first)])
+ ))
}
}🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift` around
lines 69 - 75, installActionConstructors currently serializes a missing argument
via Builtins.display(args.first ?? .null) for the builtin("open-url") and
builtin("copy-text") actions, producing the literal "nil"; instead detect when
args.first is missing or equals .null and reject the action rather than
constructing .style(.action(...)). Update the builtin handlers in
installActionConstructors (for "open-url" and "copy-text") to check args.first
and, if absent/.null, return an error/rejection value or throw so the
caller/compiler can trigger the existing fallback, otherwise build the RNAction
with the validated payload (use Builtins.display only for present/non-null
values).
| static func finish(_ base: RenderNode, _ viewName: String, _ options: [(String, LispValue)]) throws -> RenderNode { | ||
| var node = base | ||
| var frameDone = false | ||
| var borderDone = false | ||
| for (key, _) in options { | ||
| if frameKeys.contains(key) { | ||
| if !frameDone { | ||
| frameDone = true | ||
| var named: [String: RNValue] = [:] | ||
| for (k, v) in options where frameKeys.contains(k) { named[k] = try frameField(k, v, viewName) } | ||
| node = node.adding(RenderModifier("frame", named: named)) | ||
| } | ||
| continue | ||
| } | ||
| if borderKeys.contains(key) { | ||
| if !borderDone { | ||
| borderDone = true | ||
| var named: [String: RNValue] = [:] | ||
| for (k, v) in options where borderKeys.contains(k) { | ||
| named[k] = k == "border" ? .color(try Coercion.color(v, viewName)) | ||
| : .number(try Coercion.number(v, viewName)) | ||
| } | ||
| node = node.adding(RenderModifier("border", named: named)) | ||
| } | ||
| continue | ||
| } | ||
| // Re-find the value for this key (last value wins for repeats). | ||
| let value = options.last(where: { $0.0 == key })!.1 | ||
| node = try applyModifier(node, key, value, viewName) |
There was a problem hiding this comment.
Deduplicate repeated modifiers before applying the last value.
finish re-reads the last value for a repeated key, but it still walks every occurrence. For example, (text "x" :padding 1 :padding 2) emits two padding modifiers, both with 2, so SwiftUI applies padding twice instead of "last value wins." The same duplication affects other repeatable modifiers like background, opacity, and help.
Suggested fix
static func finish(_ base: RenderNode, _ viewName: String, _ options: [(String, LispValue)]) throws -> RenderNode {
var node = base
var frameDone = false
var borderDone = false
+ var seenKeys: Set<String> = []
for (key, _) in options {
+ guard seenKeys.insert(key).inserted else { continue }
+
if frameKeys.contains(key) {
if !frameDone {
frameDone = true
var named: [String: RNValue] = [:]
for (k, v) in options where frameKeys.contains(k) { named[k] = try frameField(k, v, viewName) }
@@
}
// Re-find the value for this key (last value wins for repeats).
let value = options.last(where: { $0.0 == key })!.1
node = try applyModifier(node, key, value, viewName)
}
return node
}🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift` around
lines 217 - 245, The finish(_ base:viewName:options:) function currently
iterates every option occurrence and still emits modifiers for repeated keys;
change it to deduplicate so only the last occurrence of each repeatable key is
applied. Update finish to compute the last-value map for options (ignoring keys
in frameKeys and borderKeys which are already grouped) or keep a seen set so
when iterating you skip earlier repeats and only call
applyModifier(node,key,value,viewName) for the last value; ensure frame handling
(frameKeys) and border handling (borderKeys) still build a single RenderModifier
each as before and reference the same symbol names (finish, options, frameKeys,
borderKeys, applyModifier, RenderModifier) so duplicate modifiers like
padding/background/opacity/help are emitted only once with the last provided
value.
| def("=") { args, _ in .bool(allAdjacent(args) { $0 == $1 }) } | ||
| def("not=") { args, _ in .bool(!allAdjacent(args) { $0 == $1 }) } |
There was a problem hiding this comment.
not= only checks adjacent values, so it returns the wrong result for repeated non-neighbors.
(not= 1 2 1) currently evaluates to true because allAdjacent only compares neighbors. For not= you need all values to be pairwise distinct, otherwise conditionals and filters built on it will misbehave.
Proposed fix
- def("not=") { args, _ in .bool(!allAdjacent(args) { $0 == $1 }) }
+ def("not=") { args, _ in
+ for i in 0..<args.count {
+ for j in (i + 1)..<args.count where args[i] == args[j] {
+ return .bool(false)
+ }
+ }
+ return .bool(true)
+ }📝 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.
| def("=") { args, _ in .bool(allAdjacent(args) { $0 == $1 }) } | |
| def("not=") { args, _ in .bool(!allAdjacent(args) { $0 == $1 }) } | |
| def("=") { args, _ in .bool(allAdjacent(args) { $0 == $1 }) } | |
| def("not=") { args, _ in | |
| for i in 0..<args.count { | |
| for j in (i + 1)..<args.count where args[i] == args[j] { | |
| return .bool(false) | |
| } | |
| } | |
| return .bool(true) | |
| } |
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift` around
lines 35 - 36, The current def("not=") uses allAdjacent and only compares
neighbors, so change its closure to verify pairwise distinctness across all
arguments: replace def("not=") { args, _ in .bool(!allAdjacent(args) { $0 == $1
}) } with a closure that iterates i from 0..<args.count and j from
i+1..<args.count and returns .bool(false) if any args[i] == args[j], otherwise
.bool(true); reference def("not="), allAdjacent and def("=") to locate the
related equality helpers and ensure you use the Value equality operator already
used elsewhere.
| // No PRs, ports, or progress. | ||
| #expect(node.nodes(kind: "progress-view").isEmpty) | ||
| #expect(!node.containsText("#")) |
There was a problem hiding this comment.
The “no PRs” assertion can’t catch leaked PR rows.
Line 53 checks for an exact text node equal to "#", but PR rows render strings like "#5108" and "#5099". That means this assertion stays green even if pull-request rows show up in the minimal layout. Check text-node prefixes or PR-row node structure instead.
🤖 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/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/SidebarScriptTests.swift`
around lines 51 - 53, The current assertion uses node.containsText("#") which
only matches an exact "#" and misses PR labels like "`#5108`"; update the test to
assert no PR rows by either checking that node.nodes(kind: "pr-row").isEmpty or
by scanning text nodes for a leading "#" (e.g. ensure no text node hasPrefix
"#") instead of the exact-match containsText("#"), so that any leaked PR entries
are detected; modify the assertion around node.nodes(kind: "progress-view") /
node.containsText("#") accordingly.
| guard let source = try? String(contentsOf: url, encoding: .utf8) else { | ||
| script = nil | ||
| version = 0 | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
git ls-files "Sources/SidebarScriptStore.swift" || true
# Show the relevant section with context
sed -n '1,120p' Sources/SidebarScriptStore.swift | cat -n
sed -n '120,240p' Sources/SidebarScriptStore.swift | cat -n
sed -n '240,360p' Sources/SidebarScriptStore.swift | cat -n
# Search for logger and related error-handling patterns
rg -n "logger|Logger|error\\(|log\\(" Sources/SidebarScriptStore.swift
rg -n "sidebar\\.lisp|lisp|contentsOf:\\s*url|String\\(contentsOf:.*encoding" Sources/SidebarScriptStore.swift
rg -n "try\\?\\s+String\\(contentsOf:" Sources/SidebarScriptStore.swift
# If present, locate the definition of logger or type
rg -n "struct SidebarScriptStore|class SidebarScriptStore|enum SidebarScriptStore|SidebarScriptStore" Sources/SidebarScriptStore.swiftRepository: manaflow-ai/cmux
Length of output: 9100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fileReadNoSuchFile|fileReadNoPermission|CocoaError\\.fileRead|contentsOf: url" -S Sources .github || true
rg -n "Logger\\(subsystem: \"com\\.manaflow\\.cmux\".*SidebarScript|SidebarScript" Sources || trueRepository: manaflow-ai/cmux
Length of output: 4551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,320p' Sources/Settings/ConfigSource.swift | cat -n
sed -n '320,440p' Sources/Settings/ConfigSource.swift | cat -n
sed -n '260,330p' Sources/GhosttyConfig.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 13910
Differentiate missing vs unreadable sidebar.lisp reads in SidebarScriptStore
init(url:) uses try? String(contentsOf:encoding:), which treats missing files, permission errors, and UTF-8 decoding failures the same (customization silently disabled). Keep the “file missing” case quiet, but log other read/decoding failures.
Suggested fix
- guard let source = try? String(contentsOf: url, encoding: .utf8) else {
- script = nil
- version = 0
- return
- }
+ let source: String
+ do {
+ source = try String(contentsOf: url, encoding: .utf8)
+ } catch CocoaError.fileReadNoSuchFile {
+ script = nil
+ version = 0
+ return
+ } catch {
+ script = nil
+ version = 0
+ Self.logger.error("sidebar.lisp could not be read: \(String(describing: error), privacy: .private)")
+ return
+ }🤖 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 `@Sources/SidebarScriptStore.swift` around lines 40 - 43, Replace the silent
try? in SidebarScriptStore.init(url:) with a do-catch: attempt to read
String(contentsOf: url, encoding: .utf8) and on success assign script/version as
before; if reading fails first check FileManager.default.fileExists(atPath:
url.path) and if the file does not exist keep script = nil and version = 0
silently, otherwise log the actual error (e.g., via NSLog/os_log or the
project's logger) so permission/decoding errors are reported; update the error
branch to set script/version the same as the missing-file branch while adding
the logged error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ad971d482
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def("append") { args, _ in | ||
| var out: [LispValue] = [] | ||
| for a in args { out.append(contentsOf: try listArg(a, "append")) } | ||
| return .list(out) |
There was a problem hiding this comment.
When a sidebar script combines generated lists with append (for example recursively doubling (range 4096)), this builtin copies all elements without checking Evaluator.generatedCollectionLimit or consuming steps for the emitted values. Because row rendering runs synchronously, a script can allocate/hang well before the evaluator's step/depth guards fire; please cap out.count or consume budget as elements are appended.
Useful? React with 👍 / 👎.
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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift (1)
8-18: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the new public package API with full DocC.
steps,stepLimit,depthLimit,init,eval, andapplyare public additions, but they do not have the required triple-slash DocC comments and method callouts yet.As per coding guidelines,
Every public symbol in new Swift packages under Packages/ must be documented with Swift-DocC triple-slash comment at time of writing: one-sentence summary ending with period, optional blank line and discussion paragraph,@Parameter/@Returns/@Throws callouts, examples in fenced code blocks.Also applies to: 23-25, 235-246
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift` around lines 8 - 18, Add Swift-DocC triple-slash documentation for the new public API on Evaluator: document the public class Evaluator and each public symbol steps, stepLimit, depthLimit, init(stepLimit:depthLimit:), eval(...), and apply(...) with a one-sentence summary ending with a period, an optional discussion paragraph, and the appropriate `@Parameters/`@Returns/@Throws callouts; include a short fenced code example for each method demonstrating typical usage. Ensure the docs follow the repository guideline (one-sentence summary, optional blank line, callouts, fenced examples) and add the same DocC comments for the other public symbols mentioned around lines 23-25 and 235-246 so every new public symbol in the package has triple-slash documentation.
♻️ Duplicate comments (5)
Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift (3)
219-225:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject trailing parameters after
&.
parseParams(_:)still accepts(fn (a & rest extra) ...)and silently dropsextra, so malformed signatures compile with a different arity than the script author wrote.Suggested fix
if s == "&" { guard i + 1 < forms.count, case .symbol(let r) = forms[i + 1] else { throw LispError.eval(String(localized: "sidebarScript.error.restParam", defaultValue: "'&' must be followed by one rest parameter.", bundle: .module)) } + guard i + 2 == forms.count else { + throw LispError.eval(String(localized: "sidebarScript.error.restParam", + defaultValue: "'&' must be followed by one final rest parameter.", bundle: .module)) + } rest = r break }🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift` around lines 219 - 225, In parseParams(_:) the '&' branch currently accepts a rest symbol but ignores any following parameters; change it to validate that the '&' is followed by exactly one symbol and that this symbol is the last form: after matching case .symbol(let r) and assigning rest = r, check that i + 2 == forms.count (or that there are no additional forms) and if not throw a LispError.eval with a localized message indicating '&' must be followed by a single rest parameter and be the last parameter; keep the existing localized bundle/message style and reuse the same guard/case location (the block with case .symbol(let r) and rest = r).
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
depthLimitineval, not only in closure application.Deep nesting through special forms like
if,cond, ordonever reachesapplyClosure, so this still leaves an unchecked stack-growth path despite lowering the limit to 64.Suggested fix
public func eval(_ form: LispValue, in env: LispEnvironment) throws -> LispValue { + depth += 1 + defer { depth -= 1 } + if depth > depthLimit { throw LispError.depthLimit } + steps += 1 if steps > stepLimit { throw LispError.stepLimit } switch form { @@ private func applyClosure( _ name: String, _ params: [String], _ rest: String?, _ body: [LispValue], _ closureEnv: LispEnvironment, _ args: [LispValue] ) throws -> LispValue { - depth += 1 - defer { depth -= 1 } - if depth > depthLimit { throw LispError.depthLimit } - if rest == nil && args.count != params.count { throw LispError.arity(name, expected: "\(params.count) arguments", got: args.count) }Also applies to: 258-260
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift` around lines 23 - 25, The eval function increments steps and enforces stepLimit but does not enforce the depthLimit for nested special forms; add a depth check at the start of public func eval(_ form: LispValue, in env: LispEnvironment) (using the same depth counter used elsewhere) to throw LispError.depthLimit when depth > depthLimit, ensuring deep nesting through special forms (if/cond/do) triggers the limit; also mirror this change where eval is recursively called (referencing the depth parameter/field and eval(...) itself) so applyClosure and other call sites consistently enforce depthLimit.
116-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
letstill behaves likelet*.Both forms land in the same helper, and every binding is evaluated against
scope, so laterletbindings can see earlier ones. Plainletshould evaluate RHS expressions in the outer environment.Suggested fix
- case "let", "let*": - return try evalLet(args, in: env) + case "let": + return try evalLet(args, sequential: false, in: env) + case "let*": + return try evalLet(args, sequential: true, in: env) @@ - private func evalLet(_ args: [LispValue], in env: LispEnvironment) throws -> LispValue { + private func evalLet( + _ args: [LispValue], + sequential: Bool, + in env: LispEnvironment + ) throws -> LispValue { @@ - // Sequential binding (let*): later bindings see earlier ones. - scope.define(name, try eval(pair[1], in: scope)) + let value = try eval(pair[1], in: sequential ? scope : env) + scope.define(name, value) }Also applies to: 152-166
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift` around lines 116 - 117, The switch dispatch currently sends both "let" and "let*" to evalLet, causing plain let to behave like let*; update the dispatch so "let" calls evalLet and "let*" calls a new or existing evalLetStar helper (or change evalLet to accept a sequential flag) and implement the semantics: evalLet must evaluate each binding's RHS using the outer environment (the passed-in env) and then extend the local scope with all bindings at once, while evalLetStar must evaluate each binding in sequence against the growing scope so later bindings can see earlier ones; update function names referenced (evalLet and evalLetStar) and their callers accordingly.Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift (2)
80-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing action payloads still serialize to
"nil".
(open-url)and(copy-text)currently produce a valid action with a"nil"payload instead of failing the script, so malformed actions bypass the fallback path.Suggested fix
private static func installActionConstructors(_ env: LispEnvironment) { builtin("open-url", env) { args, _ in - .style(.action(RNAction(kind: "open-url", payload: ["url": Builtins.display(args.first ?? .null)]))) + guard let first = args.first, first != .null else { + throw LispError.arity("open-url", expected: "a URL", got: args.count) + } + return .style(.action(RNAction(kind: "open-url", payload: ["url": Builtins.display(first)]))) } builtin("copy-text", env) { args, _ in - .style(.action(RNAction(kind: "copy", payload: ["text": Builtins.display(args.first ?? .null)]))) + guard let first = args.first, first != .null else { + throw LispError.arity("copy-text", expected: "a value", got: args.count) + } + return .style(.action(RNAction(kind: "copy", payload: ["text": Builtins.display(first)]))) } }🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift` around lines 80 - 86, installActionConstructors currently uses Builtins.display(args.first ?? .null) which serializes missing args to the string "nil" instead of failing; update the builtin("open-url", env) and builtin("copy-text", env) closures to explicitly validate args.first (and that it's not .null) before constructing RNAction, and if validation fails return/throw an error so the script fails and fallback runs; in other words, guard let first = args.first, first != .null (and optionally that Builtins.display(first) yields a non-empty string) and only then build RNAction(kind: "open-url", payload: ["url": Builtins.display(first)]) and RNAction(kind: "copy", payload: ["text": Builtins.display(first)]), otherwise return an error from the closure.
228-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRepeated modifiers are still emitted multiple times.
The loop re-reads the last value for a repeated key, but it still applies that modifier once per occurrence.
:padding 1 :padding 2therefore adds padding twice instead of honoring a single “last value wins” modifier.Suggested fix
static func finish(_ base: RenderNode, _ viewName: String, _ options: [(String, LispValue)]) throws -> RenderNode { var node = base var frameDone = false var borderDone = false + var seenKeys: Set<String> = [] for (key, _) in options { if frameKeys.contains(key) { if !frameDone { frameDone = true @@ } continue } + guard seenKeys.insert(key).inserted else { continue } // Re-find the value for this key (last value wins for repeats). let value = options.last(where: { $0.0 == key })!.1 node = try applyModifier(node, key, value, viewName) } return node🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift` around lines 228 - 256, The loop in finish(_:viewName:options:) applies modifiers once per occurrence, ignoring the intended "last value wins" semantics; fix by tracking which keys have been processed with a Set<String> (e.g. var seenKeys = Set<String>()) and skip any option whose key is in seenKeys at loop start. When you emit the grouped "frame" modifier (frameKeys) or "border" modifier (borderKeys) mark each involved key as seen (insert those k into seenKeys) so they aren't applied again, and for single-key modifiers use options.last(where:) to pick the last value before inserting the key into seenKeys and calling applyModifier.
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift`:
- Around line 8-18: Add Swift-DocC triple-slash documentation for the new public
API on Evaluator: document the public class Evaluator and each public symbol
steps, stepLimit, depthLimit, init(stepLimit:depthLimit:), eval(...), and
apply(...) with a one-sentence summary ending with a period, an optional
discussion paragraph, and the appropriate `@Parameters/`@Returns/@Throws callouts;
include a short fenced code example for each method demonstrating typical usage.
Ensure the docs follow the repository guideline (one-sentence summary, optional
blank line, callouts, fenced examples) and add the same DocC comments for the
other public symbols mentioned around lines 23-25 and 235-246 so every new
public symbol in the package has triple-slash documentation.
---
Duplicate comments:
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift`:
- Around line 80-86: installActionConstructors currently uses
Builtins.display(args.first ?? .null) which serializes missing args to the
string "nil" instead of failing; update the builtin("open-url", env) and
builtin("copy-text", env) closures to explicitly validate args.first (and that
it's not .null) before constructing RNAction, and if validation fails
return/throw an error so the script fails and fallback runs; in other words,
guard let first = args.first, first != .null (and optionally that
Builtins.display(first) yields a non-empty string) and only then build
RNAction(kind: "open-url", payload: ["url": Builtins.display(first)]) and
RNAction(kind: "copy", payload: ["text": Builtins.display(first)]), otherwise
return an error from the closure.
- Around line 228-256: The loop in finish(_:viewName:options:) applies modifiers
once per occurrence, ignoring the intended "last value wins" semantics; fix by
tracking which keys have been processed with a Set<String> (e.g. var seenKeys =
Set<String>()) and skip any option whose key is in seenKeys at loop start. When
you emit the grouped "frame" modifier (frameKeys) or "border" modifier
(borderKeys) mark each involved key as seen (insert those k into seenKeys) so
they aren't applied again, and for single-key modifiers use options.last(where:)
to pick the last value before inserting the key into seenKeys and calling
applyModifier.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift`:
- Around line 219-225: In parseParams(_:) the '&' branch currently accepts a
rest symbol but ignores any following parameters; change it to validate that the
'&' is followed by exactly one symbol and that this symbol is the last form:
after matching case .symbol(let r) and assigning rest = r, check that i + 2 ==
forms.count (or that there are no additional forms) and if not throw a
LispError.eval with a localized message indicating '&' must be followed by a
single rest parameter and be the last parameter; keep the existing localized
bundle/message style and reuse the same guard/case location (the block with case
.symbol(let r) and rest = r).
- Around line 23-25: The eval function increments steps and enforces stepLimit
but does not enforce the depthLimit for nested special forms; add a depth check
at the start of public func eval(_ form: LispValue, in env: LispEnvironment)
(using the same depth counter used elsewhere) to throw LispError.depthLimit when
depth > depthLimit, ensuring deep nesting through special forms (if/cond/do)
triggers the limit; also mirror this change where eval is recursively called
(referencing the depth parameter/field and eval(...) itself) so applyClosure and
other call sites consistently enforce depthLimit.
- Around line 116-117: The switch dispatch currently sends both "let" and "let*"
to evalLet, causing plain let to behave like let*; update the dispatch so "let"
calls evalLet and "let*" calls a new or existing evalLetStar helper (or change
evalLet to accept a sequential flag) and implement the semantics: evalLet must
evaluate each binding's RHS using the outer environment (the passed-in env) and
then extend the local scope with all bindings at once, while evalLetStar must
evaluate each binding in sequence against the growing scope so later bindings
can see earlier ones; update function names referenced (evalLet and evalLetStar)
and their callers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba96e07e-7172-4519-b741-c5659e1a484c
📒 Files selected for processing (2)
Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift
| var out: [LispValue] = [] | ||
| var x = start | ||
| if step > 0 { while x < end { out.append(.int(Int(x))); x += step } } | ||
| else { while x > end { out.append(.int(Int(x))); x += step } } |
There was a problem hiding this comment.
range truncates fractional steps to integers
Medium Severity
The range builtin unconditionally wraps each element with .int(Int(x)), which truncates fractional values toward zero. A call like (range 0 1 0.3) produces [0, 0, 0, 0] instead of [0, 0.3, 0.6, 0.9]. Every other arithmetic builtin uses LispValue.number(...) which correctly preserves fractional values as .double while collapsing integral values to .int. Using .number(x) here would fix the issue without changing behavior for integer ranges.
Reviewed by Cursor Bugbot for commit 2ad971d. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d32d050e3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .eval(String( | ||
| localized: "sidebarScript.error.unbound", | ||
| defaultValue: "Unknown name '\(name)'.", | ||
| bundle: .module |
There was a problem hiding this comment.
These new String(localized:..., bundle: .module) errors are user-facing when sidebar.lisp fails, but the new package only adds DefaultSidebar.lisp under its module resources and no Localizable.xcstrings, so the app-level catalog cannot supply the required Japanese translations. This violates /workspace/cmux/AGENTS.md (“Keys go in Resources/Localizable.xcstrings with translations for all supported languages”) and leaves script error UI in English for non-English users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift`:
- Around line 27-31: The public method consumeStep(_:) is undocumented; add
Swift-DocC comments to the public symbol explaining its purpose, when callers
should invoke it, the meaning of the count parameter, and the semantics of the
thrown error; specifically document the parameter `count`, that it increments
internal `steps` and compares against `stepLimit`, and that it throws
`LispError.stepLimit` when the limit is exceeded (also note behavior for
non-positive counts). Place the doc comment immediately above the `public func
consumeStep(_ count: Int = 1) throws` declaration in Evaluator.swift.
In `@Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift`:
- Around line 35-37: Add a Swift-DocC triple-slash comment above the public
method freeze() in the LispEnvironment type describing its purpose (it makes the
environment immutable by setting isMutable to false), the observable effect on
subsequent operations, any thread-safety or usage notes, and the API
availability/semantics; ensure the comment is written as standard Swift-DocC
(///) and references freeze() and isMutable so readers understand that calling
freeze() flips isMutable to false and prevents further mutations.
- Around line 6-10: Add a Swift-DocC triple-slash documentation comment for the
public enum SetResult: provide a one-sentence summary ending with a period
describing what the enum represents, optionally a short discussion paragraph,
and document the cases (assigned, missing, immutable) with brief explanations;
attach the comment immediately above the declaration of SetResult so the public
API is documented per package guidelines.
- Around line 16-19: Document the public initializer
LispEnvironment.init(parent:isMutable:) by adding a Swift doc comment that
describes the purpose of the initializer and the isMutable parameter; explain
what passing false does (e.g., creates an immutable environment that prevents
modifications to vars), when callers should pass false vs the default true, and
any observable effects or errors when attempting to mutate an immutable
environment; place the comment directly above the public init declaration and
use the standard Swift /// parameter tags to describe parent and isMutable.
🪄 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: 55e040d2-c8c4-4c1f-a597-8a213478d48c
📒 Files selected for processing (9)
Packages/CmuxSidebarScript/README.mdPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Errors.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swiftPackages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/EvaluatorTests.swiftPackages/CmuxSidebarScript/Tests/CmuxSidebarScriptTests/SidebarScriptTests.swiftSources/ContentView.swift
| public func consumeStep(_ count: Int = 1) throws { | ||
| guard count > 0 else { return } | ||
| steps += count | ||
| if steps > stepLimit { throw LispError.stepLimit } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the public consumeStep(_:) method.
consumeStep(_:) is a new public method but lacks Swift-DocC documentation explaining its purpose, when it should be called, and what it throws. As per coding guidelines, public methods require complete documentation including @Parameter and @Throws callouts.
📝 Add documentation
+/// Consumes evaluation steps from the budget.
+///
+/// Call this before performing work proportional to `count` (such as iterating over
+/// a collection). The evaluator tracks cumulative steps across the entire render pass.
+///
+/// `@Parameter` count The number of steps to consume; must be positive.
+/// `@Throws` `LispError.stepLimit` when the cumulative step count exceeds `stepLimit`.
public func consumeStep(_ count: Int = 1) throws {As per coding guidelines: "Every public symbol in new Swift packages under Packages/ must be documented..."
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift` around
lines 27 - 31, The public method consumeStep(_:) is undocumented; add Swift-DocC
comments to the public symbol explaining its purpose, when callers should invoke
it, the meaning of the count parameter, and the semantics of the thrown error;
specifically document the parameter `count`, that it increments internal `steps`
and compares against `stepLimit`, and that it throws `LispError.stepLimit` when
the limit is exceeded (also note behavior for non-positive counts). Place the
doc comment immediately above the `public func consumeStep(_ count: Int = 1)
throws` declaration in Evaluator.swift.
| public enum SetResult { | ||
| case assigned | ||
| case missing | ||
| case immutable | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the public SetResult enum.
SetResult is a new public API but lacks the required Swift-DocC documentation. As per coding guidelines, "Every public symbol in new Swift packages under Packages/ must be documented with Swift-DocC triple-slash comment at time of writing: one-sentence summary ending with period..."
📝 Add documentation
+/// The result of attempting to update an existing binding.
public enum SetResult {
+ /// The binding was successfully updated.
case assigned
+ /// The binding does not exist in this environment or any parent.
case missing
+ /// The binding exists but the environment has been frozen.
case immutable
}As per coding guidelines: "Every public symbol in new Swift packages under Packages/ must be documented with Swift-DocC triple-slash comment at time of writing: one-sentence summary ending with period, optional blank line and discussion paragraph, @Parameter/@Returns/@throws callouts, examples in fenced code blocks."
📝 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.
| public enum SetResult { | |
| case assigned | |
| case missing | |
| case immutable | |
| } | |
| /// The result of attempting to update an existing binding. | |
| public enum SetResult { | |
| /// The binding was successfully updated. | |
| case assigned | |
| /// The binding does not exist in this environment or any parent. | |
| case missing | |
| /// The binding exists but the environment has been frozen. | |
| case immutable | |
| } |
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift`
around lines 6 - 10, Add a Swift-DocC triple-slash documentation comment for the
public enum SetResult: provide a one-sentence summary ending with a period
describing what the enum represents, optionally a short discussion paragraph,
and document the cases (assigned, missing, immutable) with brief explanations;
attach the comment immediately above the declaration of SetResult so the public
API is documented per package guidelines.
| public init(parent: LispEnvironment? = nil, isMutable: Bool = true) { | ||
| self.vars = [:] | ||
| self.isMutable = isMutable | ||
| self.parent = parent |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the isMutable parameter in the public initializer.
The public init now accepts an isMutable parameter but lacks documentation explaining when and why a caller would pass false. As per coding guidelines, public symbols require complete documentation including parameter descriptions.
📝 Add parameter documentation
+/// Creates a new lexical environment.
+///
+/// `@Parameter` parent The parent environment for scope chaining, or `nil` for a top-level environment.
+/// `@Parameter` isMutable When `false`, `set(_:_:)` will return `.immutable` for any binding in this scope.
public init(parent: LispEnvironment? = nil, isMutable: Bool = true) {As per coding guidelines: "Every public symbol in new Swift packages under Packages/ must be documented..."
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift`
around lines 16 - 19, Document the public initializer
LispEnvironment.init(parent:isMutable:) by adding a Swift doc comment that
describes the purpose of the initializer and the isMutable parameter; explain
what passing false does (e.g., creates an immutable environment that prevents
modifications to vars), when callers should pass false vs the default true, and
any observable effects or errors when attempting to mutate an immutable
environment; place the comment directly above the public init declaration and
use the standard Swift /// parameter tags to describe parent and isMutable.
| public func freeze() { | ||
| isMutable = false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the public freeze() method.
freeze() is a new public method but lacks documentation explaining its purpose and effect. As per coding guidelines, every public symbol requires Swift-DocC documentation.
📝 Add method documentation
+/// Makes this environment immutable.
+///
+/// After freezing, `set(_:_:)` will return `.immutable` when attempting to update
+/// bindings in this scope. Child environments and parent environments are unaffected.
public func freeze() {As per coding guidelines: "Every public symbol in new Swift packages under Packages/ must be documented with Swift-DocC triple-slash comment at time of writing..."
📝 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.
| public func freeze() { | |
| isMutable = false | |
| } | |
| /// Makes this environment immutable. | |
| /// | |
| /// After freezing, `set(_:_:)` will return `.immutable` when attempting to update | |
| /// bindings in this scope. Child environments and parent environments are unaffected. | |
| public func freeze() { | |
| isMutable = false | |
| } |
🤖 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/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift`
around lines 35 - 37, Add a Swift-DocC triple-slash comment above the public
method freeze() in the LispEnvironment type describing its purpose (it makes the
environment immutable by setting isMutable to false), the observable effect on
subsequent operations, any thread-safety or usage notes, and the API
availability/semantics; ensure the comment is written as standard Swift-DocC
(///) and references freeze() and isMutable so readers understand that calling
freeze() flips isMutable to false and prevents further mutations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10517ade2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defaultValue: "The script must define a 'render-row' function.", | ||
| bundle: .module)) | ||
| } | ||
| baseEnv.freeze() |
There was a problem hiding this comment.
Only baseEnv is frozen here, so any mutable child environment captured during top-level evaluation remains writable across row renders. For example, (def next (let ((counter 0)) (fn () (set! counter (+ counter 1)) counter))) lets render-row mutate counter on each row, making identical SidebarScriptContext inputs produce different RenderNodes depending on render order despite the package's deterministic/equatable contract. Please recursively freeze captured scopes or make child scopes inherit the immutable state after compilation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dee6322db5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch args[0] { | ||
| case .map(let m): return m[keyName(args[1])] ?? fallback | ||
| case .list(let l): | ||
| let i = Int((try? num(args[1], "get")) ?? -1) |
There was a problem hiding this comment.
When a custom sidebar script indexes a list with the exposed infinity constant (for example (get (list "a") infinity "fallback")) or a NaN value, this Int(Double) conversion traps before get can return the fallback or throw a render error. Because row rendering runs in the app fallback path, that turns a malformed sidebar.lisp into a process crash instead of a native-row fallback; please reuse the finite integer validation used by other index-like builtins before converting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Sources/ContentView.swift`:
- Around line 14609-14683: The SidebarWorkspaceRowChrome view is large and
should be extracted into its own Swift file: create a new file under Sources
(e.g., SidebarWorkspaceRowChrome.swift), add required imports (import SwiftUI),
copy the entire private struct SidebarWorkspaceRowChrome<Content: View> { ... }
into it, preserve its generic signature, properties, body and the closeWorkspace
closure, and keep the call site in ContentView.swift unchanged; if the struct
referenced any fileprivate/internal symbols from ContentView.swift, adjust their
access (make them internal/public or pass them in as parameters) so the
extracted file compiles, then remove the struct definition from
ContentView.swift. Ensure the new file uses the same access level (private ->
internal if needed across files) and build.
🪄 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: b04620db-500d-4d3e-87b3-ee803fc0257f
📒 Files selected for processing (1)
Sources/ContentView.swift
| private struct SidebarWorkspaceRowChrome<Content: View>: View { | ||
| let content: Content | ||
| let latestLog: SidebarLogEntry? | ||
| let progress: SidebarProgressState? | ||
| let metadataBlockCount: Int | ||
| let backgroundColor: Color | ||
| let activeBorderColor: Color | ||
| let activeBorderLineWidth: CGFloat | ||
| let showsLeadingRail: Bool | ||
| let railColor: Color | ||
| let showsWorkspaceShortcutHint: Bool | ||
| let workspaceShortcutLabel: String? | ||
| let shortcutHintEmphasis: Double | ||
| let sidebarShortcutHintXOffset: Double | ||
| let sidebarShortcutHintYOffset: Double | ||
| let shortcutHintFontSize: CGFloat | ||
| let showCloseButton: Bool | ||
| let closeButtonTooltip: String | ||
| let closeButtonFontSize: CGFloat | ||
| let closeButtonForegroundColor: Color | ||
| let closeButtonWidth: CGFloat | ||
| let closeButtonHitSize: CGFloat | ||
| let closeWorkspace: () -> Void | ||
|
|
||
| var body: some View { | ||
| content | ||
| .animation(.easeInOut(duration: 0.2), value: latestLog) | ||
| .animation(.easeInOut(duration: 0.2), value: progress != nil) | ||
| .animation(.easeInOut(duration: 0.2), value: metadataBlockCount) | ||
| .padding(.horizontal, 10) | ||
| .padding(.vertical, 8) | ||
| .background( | ||
| RoundedRectangle(cornerRadius: 6) | ||
| .fill(backgroundColor) | ||
| .overlay { | ||
| RoundedRectangle(cornerRadius: 6) | ||
| .strokeBorder(activeBorderColor, lineWidth: activeBorderLineWidth) | ||
| } | ||
| .overlay(alignment: .leading) { | ||
| if showsLeadingRail { | ||
| Capsule(style: .continuous) | ||
| .fill(railColor) | ||
| .frame(width: 3) | ||
| .padding(.leading, 4) | ||
| .padding(.vertical, 5) | ||
| .offset(x: -1) | ||
| } | ||
| } | ||
| ) | ||
| .sidebarShortcutHintOverlay( | ||
| text: showsWorkspaceShortcutHint ? workspaceShortcutLabel : nil, | ||
| emphasis: shortcutHintEmphasis, | ||
| offsetX: sidebarShortcutHintXOffset, | ||
| offsetY: sidebarShortcutHintYOffset, | ||
| fontSize: shortcutHintFontSize | ||
| ) | ||
| .overlay(alignment: .topTrailing) { | ||
| if showsWorkspaceShortcutHint { | ||
| EmptyView() | ||
| } else if showCloseButton { | ||
| Button(action: closeWorkspace) { | ||
| Image(systemName: "xmark") | ||
| .font(.system(size: closeButtonFontSize, weight: .medium)) | ||
| .foregroundColor(closeButtonForegroundColor) | ||
| } | ||
| .buttonStyle(.plain) | ||
| .safeHelp(closeButtonTooltip) | ||
| .frame(width: closeButtonWidth, height: closeButtonHitSize, alignment: .center) | ||
| .padding(.top, 8) | ||
| .padding(.trailing, 10) | ||
| } | ||
| } | ||
| .shortcutHintVisibilityAnimation(value: showsWorkspaceShortcutHint) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract SidebarWorkspaceRowChrome into its own Swift file.
Sources/ContentView.swift is already one of the repo’s oversized view files, and adding another helper view here keeps pushing against the CI file-length budget. Please move this wrapper into a dedicated file under Sources/ and keep only the call site in ContentView.swift.
Based on learnings, "This repo’s CI enforces a Swift file-length budget for large view files (e.g., Sources/ContentView.swift). When adding helper views/small components, avoid bloating the existing file: extract the subview into a dedicated Swift file under Sources..."
🤖 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 `@Sources/ContentView.swift` around lines 14609 - 14683, The
SidebarWorkspaceRowChrome view is large and should be extracted into its own
Swift file: create a new file under Sources (e.g.,
SidebarWorkspaceRowChrome.swift), add required imports (import SwiftUI), copy
the entire private struct SidebarWorkspaceRowChrome<Content: View> { ... } into
it, preserve its generic signature, properties, body and the closeWorkspace
closure, and keep the call site in ContentView.swift unchanged; if the struct
referenced any fileprivate/internal symbols from ContentView.swift, adjust their
access (make them internal/public or pass them in as parameters) so the
extracted file compiles, then remove the struct definition from
ContentView.swift. Ensure the new file uses the same access level (private ->
internal if needed across files) and build.
There was a problem hiding this comment.
4 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="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:15452">
P2: Native row builds every time, even when script row is used. Wastes render work on each sidebar row. Build only the selected branch.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:315">
P1: The new `intValue` validation (finite-check before `Int(Double)`) was applied to `nth`, `substring`, and `pad`, but the `get` builtin still uses a raw `Int(Double)` conversion. Passing `infinity` or `NaN` as an index to `get` will trap the process instead of throwing a recoverable `LispError`, turning a malformed script into a crash rather than a native-row fallback.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:320">
P2: Integer parser not enforce integer. Fractional input gets silently truncated. Reject non-whole numbers before converting.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift:36">
P2: Freeze only locks one env node. Captured child scopes from compile stay mutable, so `set!` can keep cross-row state. Freeze whole compile-time env graph or block writes to captured compile-time scopes too.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,343 @@ | |||
| import Foundation | |||
There was a problem hiding this comment.
P1: The new intValue validation (finite-check before Int(Double)) was applied to nth, substring, and pad, but the get builtin still uses a raw Int(Double) conversion. Passing infinity or NaN as an index to get will trap the process instead of throwing a recoverable LispError, turning a malformed script into a crash rather than a native-row fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift, line 315:
<comment>The new `intValue` validation (finite-check before `Int(Double)`) was applied to `nth`, `substring`, and `pad`, but the `get` builtin still uses a raw `Int(Double)` conversion. Passing `infinity` or `NaN` as an index to `get` will trap the process instead of throwing a recoverable `LispError`, turning a malformed script into a crash rather than a native-row fallback.</comment>
<file context>
@@ -279,16 +299,27 @@ enum Builtins {
return .string(left ? padding + s : s + padding)
}
+ private static func intValue(_ v: LispValue, _ form: String) throws -> Int {
+ let n = try num(v, form)
+ guard n.isFinite, n >= Double(Int.min), n <= Double(Int.max) else {
</file context>
| guard n.isFinite, n >= Double(Int.min), n <= Double(Int.max) else { | ||
| throw LispError.type(form, expected: "a finite integer", got: v) | ||
| } | ||
| return Int(n) |
There was a problem hiding this comment.
P2: Integer parser not enforce integer. Fractional input gets silently truncated. Reject non-whole numbers before converting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift, line 320:
<comment>Integer parser not enforce integer. Fractional input gets silently truncated. Reject non-whole numbers before converting.</comment>
<file context>
@@ -279,16 +299,27 @@ enum Builtins {
+ guard n.isFinite, n >= Double(Int.min), n <= Double(Int.max) else {
+ throw LispError.type(form, expected: "a finite integer", got: v)
+ }
+ return Int(n)
+ }
+
</file context>
| return Int(n) | |
| guard n.rounded(.towardZero) == n else { | |
| throw LispError.type(form, expected: "a finite integer", got: v) | |
| } | |
| return Int(n) |
| } | ||
|
|
||
| public func freeze() { | ||
| isMutable = false |
There was a problem hiding this comment.
P2: Freeze only locks one env node. Captured child scopes from compile stay mutable, so set! can keep cross-row state. Freeze whole compile-time env graph or block writes to captured compile-time scopes too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift, line 36:
<comment>Freeze only locks one env node. Captured child scopes from compile stay mutable, so `set!` can keep cross-row state. Freeze whole compile-time env graph or block writes to captured compile-time scopes too.</comment>
<file context>
@@ -24,17 +32,22 @@ public final class LispEnvironment {
}
+ public func freeze() {
+ isMutable = false
+ }
+
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45cd26edb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| at: url.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| try source.write(to: url, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
When a user already has a hand-edited ~/.config/cmux/sidebar.lisp, choosing any bundled layout from the new menu calls applyDemo and this write replaces that file with no confirmation or backup, so a single exploratory click can permanently destroy their custom sidebar. Please either confirm destructive changes, keep demos in a separate selection state, or save/restore the previous script before overwriting it.
Useful? React with 👍 / 👎.
| switch action.kind { | ||
| case "open-url": | ||
| guard let raw = action.payload["url"], let url = URL(string: raw) else { return } | ||
| if tabManager.openBrowser(inWorkspace: tab.id, url: url, insertAtEnd: true) == nil { |
There was a problem hiding this comment.
For the bundled/default Lisp rows, PR and port chips use (open-url ...), but this handler always opens through tabManager.openBrowser(...) first, bypassing the existing openSidebarPullRequestLinksInCmuxBrowser / openSidebarPortLinksInCmuxBrowser settings that native rows enforce in openPullRequestLink and openPortLink. In profiles where those settings are disabled, switching to a scripted layout changes sidebar link behavior; route these actions through the shared native link paths or carry enough action type to apply the same settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
5 issues found across 12 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Resources/LiquidGlassSidebar.lisp">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Resources/LiquidGlassSidebar.lisp:89">
P3: Optional metadata row always renders. Creates empty vertical gap when branch and remote are both missing. Wrap row with one guard.</violation>
</file>
<file name="Sources/SidebarScriptStore.swift">
<violation number="1" location="Sources/SidebarScriptStore.swift:13">
P2: State model uses ObservableObject/@Published/Combine. Repo rules for this file forbid this pattern. Switch to @Observable-style state.</violation>
<violation number="2" location="Sources/SidebarScriptStore.swift:86">
P1: Applying a demo overwrites `~/.config/cmux/sidebar.lisp` unconditionally, which can destroy a user’s existing hand-edited script. Preserve the previous file (or confirm before replacing) before writing the demo source.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptDemo.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptDemo.swift:37">
P3: `source(resourceName:)` returns "" on missing/unreadable resource without logging. When resources are missing, all demos get empty source, so `matchingDemoId("")` returns "default" — a false positive. Log the error and consider returning `nil`/`?` to let callers distinguish "no demo" from "resource missing".</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Resources/AgentOpsSidebar.lisp">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Resources/AgentOpsSidebar.lisp:53">
P2: Using a generic `open-url` action for PR links drops link-type context, so scripted sidebar rows can’t apply the same PR/port browser routing preferences as native rows. Route these through typed actions (or native link handlers) to keep behavior consistent with user settings.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| at: url.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| try source.write(to: url, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
P1: Applying a demo overwrites ~/.config/cmux/sidebar.lisp unconditionally, which can destroy a user’s existing hand-edited script. Preserve the previous file (or confirm before replacing) before writing the demo source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/SidebarScriptStore.swift, line 86:
<comment>Applying a demo overwrites `~/.config/cmux/sidebar.lisp` unconditionally, which can destroy a user’s existing hand-edited script. Preserve the previous file (or confirm before replacing) before writing the demo source.</comment>
<file context>
@@ -37,21 +39,72 @@ final class SidebarScriptStore {
+ at: url.deletingLastPathComponent(),
+ withIntermediateDirectories: true
+ )
+ try source.write(to: url, atomically: true, encoding: .utf8)
+ setSource(source, compiledScript: compiledScript)
+ Self.logger.info("Applied sidebar Lisp demo '\(logName, privacy: .public)' (\(source.count) chars).")
</file context>
| try source.write(to: url, atomically: true, encoding: .utf8) | |
| if FileManager.default.fileExists(atPath: url.path) { | |
| let backupURL = url.deletingLastPathComponent().appendingPathComponent("sidebar.lisp.bak") | |
| if FileManager.default.fileExists(atPath: backupURL.path) { | |
| try? FileManager.default.removeItem(at: backupURL) | |
| } | |
| try FileManager.default.copyItem(at: url, to: backupURL) | |
| } | |
| try source.write(to: url, atomically: true, encoding: .utf8) |
| /// it; any compile or render fault falls back to the native row, so a broken | ||
| /// script can never break the sidebar. | ||
| @MainActor | ||
| final class SidebarScriptStore: ObservableObject { |
There was a problem hiding this comment.
P2: State model uses ObservableObject/@Published/Combine. Repo rules for this file forbid this pattern. Switch to @Observable-style state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/SidebarScriptStore.swift, line 13:
<comment>State model uses ObservableObject/@Published/Combine. Repo rules for this file forbid this pattern. Switch to @Observable-style state.</comment>
<file context>
@@ -1,28 +1,30 @@
-/// reload is a deliberate follow-up.
@MainActor
-final class SidebarScriptStore {
+final class SidebarScriptStore: ObservableObject {
/// The compiled script, or nil when the user has no `sidebar.lisp` (or it
/// failed to compile).
</file context>
| private static func source(resourceName: String) -> String { | ||
| guard let url = Bundle.module.url(forResource: resourceName, withExtension: "lisp"), | ||
| let text = try? String(contentsOf: url, encoding: .utf8) else { | ||
| return "" |
There was a problem hiding this comment.
P3: source(resourceName:) returns "" on missing/unreadable resource without logging. When resources are missing, all demos get empty source, so matchingDemoId("") returns "default" — a false positive. Log the error and consider returning nil/? to let callers distinguish "no demo" from "resource missing".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptDemo.swift, line 37:
<comment>`source(resourceName:)` returns "" on missing/unreadable resource without logging. When resources are missing, all demos get empty source, so `matchingDemoId("")` returns "default" — a false positive. Log the error and consider returning `nil`/`?` to let callers distinguish "no demo" from "resource missing".</comment>
<file context>
@@ -0,0 +1,45 @@
+ private static func source(resourceName: String) -> String {
+ guard let url = Bundle.module.url(forResource: resourceName, withExtension: "lisp"),
+ let text = try? String(contentsOf: url, encoding: .utf8) else {
+ return ""
+ }
+ return text
</file context>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
1 issue found across 4 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/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Value.swift:87">
P2: Double equality here breaks on NaN. Same value can compare unequal and force false change detection. Handle NaN explicitly.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:14993">
P2: Per-row render failure logs spam fast. One bad script can flood logs on every row re-render. Gate or throttle this log.</violation>
<violation number="2" location="Sources/ContentView.swift:15013">
P2: `isDraft` is hardcoded to `false` here, so the `:draft` field in the workspace record passed to scripts is never true. The bundled `DefaultSidebar.lisp` checks `(get pr :draft)` in `pr-color` to render draft PRs gray, but that branch can never trigger—draft PRs always appear green. Plumb the actual draft status from the PR row data.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift:159">
P1: `Int(m.first?.number ?? 1)` will crash with a fatal trap if the number is `.infinity` or `.nan` (e.g. a script using `(text "x" :line-limit infinity)`). The bridge exposes `infinity` as a constant, so this is easily reachable. Clamp or reject non-finite values before the `Int` conversion to prevent an unrecoverable crash that bypasses the native-row fallback.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/RenderNodeView.swift:273">
P2: Bad hex color becomes transparent. Script typo can hide row content and mask the fault. Use a visible fallback or fail color validation earlier.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:22">
P1: Step budget only ticks in `eval`. Builtin loops can run huge work without spending steps, so script can still block UI. Add budget checks in function application and builtin iteration paths.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:162">
P2: `let` evaluates each binding value in `scope` (the child env), giving it `let*` semantics where later bindings can reference earlier ones. Standard `let` should evaluate all RHS expressions in the outer `env` before defining any names. Either fix by evaluating in `env` instead of `scope`, or collapse `let`/`let*` into one form and document the sequential behavior.</violation>
<violation number="3" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Evaluator.swift:221">
P2: Rest-param parse ignores extra tokens after `&`. Bad signatures compile silently and drop parameters. Reject anything after the rest name.</violation>
</file>
<file name="Sources/SidebarScriptStore.swift">
<violation number="1" location="Sources/SidebarScriptStore.swift:13">
P2: State model uses ObservableObject/@Published/Combine. Repo rules for this file forbid this pattern. Switch to @Observable-style state.</violation>
<violation number="2" location="Sources/SidebarScriptStore.swift:30">
P1: Error text logged as public. Can leak user script data in logs. Log private/redacted error details instead.</violation>
<violation number="3" location="Sources/SidebarScriptStore.swift:40">
P2: MainActor init does sync file read + compile. Can stall sidebar/UI startup. Move load/compile off main actor, then publish result back on MainActor.</violation>
<violation number="4" location="Sources/SidebarScriptStore.swift:86">
P1: Applying a demo overwrites `~/.config/cmux/sidebar.lisp` unconditionally, which can destroy a user’s existing hand-edited script. Preserve the previous file (or confirm before replacing) before writing the demo source.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift:95">
P2: Unknown string escape drops backslash. Parser changes user text silently. Keep backslash or throw read error.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Reader.swift:167">
P3: Dangling quote loses line number. Error points less clearly to bad script. Guard missing quoted form and throw with quote line.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift:47">
P2: Arity check too loose for rgb/rgba. Extra args get ignored silently. Enforce exact arg count.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Bridge.swift:221">
P2: Repeated modifier key applied multiple times. Last value picked, but modifier still stacks. Skip duplicate keys before apply.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:170">
P2: `record` drops a dangling key silently. Bad input should throw, not partially apply.</violation>
<violation number="2" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:315">
P1: The new `intValue` validation (finite-check before `Int(Double)`) was applied to `nth`, `substring`, and `pad`, but the `get` builtin still uses a raw `Int(Double)` conversion. Passing `infinity` or `NaN` as an index to `get` will trap the process instead of throwing a recoverable `LispError`, turning a malformed script into a crash rather than a native-row fallback.</violation>
<violation number="3" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/Builtins.swift:320">
P2: Integer parser not enforce integer. Fractional input gets silently truncated. Reject non-whole numbers before converting.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScript.swift:60">
P2: Resource load failure gets swallowed. Empty fallback hides real default-script error. Return/throw explicit load failure.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/LispEnvironment.swift:36">
P2: Freeze only locks one env node. Captured child scopes from compile stay mutable, so `set!` can keep cross-row state. Freeze whole compile-time env graph or block writes to captured compile-time scopes too.</violation>
</file>
<file name="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptDemo.swift">
<violation number="1" location="Packages/CmuxSidebarScript/Sources/CmuxSidebarScript/SidebarScriptDemo.swift:37">
P3: `source(resourceName:)` returns "" on missing/unreadable resource without logging. When resources are missing, all demos get empty source, so `matchingDemoId("")` returns "default" — a false positive. Log the error and consider returning `nil`/`?` to let callers distinguish "no demo" from "resource missing".</violation>
</file>
<file name="Sources/Update/UpdateTitlebarAccessory.swift">
<violation number="1" location="Sources/Update/UpdateTitlebarAccessory.swift:943">
P2: Right-click on the sidebar toggle no longer opens extension-provider selection, regressing that existing entrypoint for users of experimental extension sidebars.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }, | ||
| rightClickAction: { anchorView, event in | ||
| CmuxExtensionSidebarSelection.showMenu(anchorView: anchorView, event: event) | ||
| SidebarScriptLayoutMenuController.showMenu( |
There was a problem hiding this comment.
P2: Right-click on the sidebar toggle no longer opens extension-provider selection, regressing that existing entrypoint for users of experimental extension sidebars.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Update/UpdateTitlebarAccessory.swift, line 943:
<comment>Right-click on the sidebar toggle no longer opens extension-provider selection, regressing that existing entrypoint for users of experimental extension sidebars.</comment>
<file context>
@@ -940,7 +940,11 @@ struct TitlebarControlsView: View {
},
rightClickAction: { anchorView, event in
- CmuxExtensionSidebarSelection.showMenu(anchorView: anchorView, event: event)
+ SidebarScriptLayoutMenuController.showMenu(
+ scriptStore: .shared,
+ anchorView: anchorView,
</file context>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faef184379
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SidebarScriptLayoutMenuController.showMenu( | ||
| scriptStore: .shared, | ||
| anchorView: anchorView, | ||
| event: event | ||
| ) |
There was a problem hiding this comment.
When the experimental Extensions feature is enabled, the sidebar toggle's right-click action was the only pointer-based entry point to CmuxExtensionSidebarSelection.showMenu; this replacement means right-clicking the titlebar sidebar button now opens the Lisp layout menu instead, so users can no longer switch between the default and hosted extension sidebar from that workflow. I checked the remaining references: provider switching still exists via command-palette registration, but both titlebar and minimal-mode right-click call sites now bypass the provider menu.
Useful? React with 👍 / 👎.
| let columnCount = max(1, Int(node.content["columns"]?.number ?? 2)) | ||
| let spacing = CGFloat(node.content["spacing"]?.number ?? 4) | ||
| let columns = Array(repeating: GridItem(.flexible(), spacing: spacing), count: columnCount) |
There was a problem hiding this comment.
When a custom sidebar.lisp renders (grid :columns 1000000000 ...) this converts the script value directly to an Int and then allocates that many GridItems on the main-thread render path, so a malformed script can hang or OOM before the native-row fallback can help. The same line also traps for non-finite values such as the exposed infinity constant, because the conversion happens in RenderNodeView after sidebarScript.render has already returned.
Useful? React with 👍 / 👎.
|
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcf4534e8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SidebarScriptContext( | ||
| title: snapshot.title, |
There was a problem hiding this comment.
For render-row scripts this context omits id/index, so (select-workspace ws) and (close-workspace ws) generate an empty action id even though the host action handler requires a UUID payload. This means any custom row that uses the documented workspace actions on its ws record silently does nothing (or consumes the click inside a button) while whole-sidebar scripts work because their context is populated with ids.
Useful? React with 👍 / 👎.
| private func setSource(_ source: String?, compiledScript: SidebarScript?) { | ||
| self.source = source | ||
| script = compiledScript | ||
| version = source?.hashValue ?? 0 |
There was a problem hiding this comment.
version is set to source?.hashValue ?? 0, which uses 0 as the "no-script" sentinel. If a script's source has hashValue == 0 (or two different scripts share the same hash), sidebarScriptVersion won't change in the TabItemView.== comparison. Because sidebarScript is explicitly excluded from ==, version is the sole signal that forces a row re-render on script change. A false-equal version means all rows remain frozen with stale native content even though a live script is active. An incrementing counter (version += 1 on every setSource call with a non-nil source, starting from 1) eliminates the sentinel collision and is the standard approach for generation-based invalidation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd0a438c04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| default: | ||
| break |
There was a problem hiding this comment.
When a custom render-row uses any of the documented non-URL actions, such as (button :action (open-workspace file) ...) or (set-sidebar-state ...), the tap reaches this row-level handler and is dropped by the default branch. The whole-sidebar handler above implements these action kinds, so row scripts and whole-sidebar scripts now behave differently even though they share the same bridge action constructors; please share the same action dispatch or add the missing cases here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dd0a438. Configure here.
| minHeight: named["min-height"]?.number.map { CGFloat($0) }, | ||
| maxHeight: named["max-height"]?.number.map { CGFloat($0) }, | ||
| alignment: align)) | ||
| } |
There was a problem hiding this comment.
Frame modifier drops flexible options when fixed present
High Severity
When a view mixes fixed frame options (:width/:height) with flexible ones (:max-width/:min-height/etc.), Bridge.finish coalesces all frameKeys into a single "frame" modifier, but RenderNodeView.frame() checks if w != nil || h != nil and enters the fixed-dimension branch, silently dropping all min-/max- options. The bundled LiquidGlassSidebar.lisp demo uses :max-width infinity :height 88 (line 8) and :height 84 :max-width infinity (lines 11–12), so the :max-width infinity is discarded and those views won't expand to fill available width, breaking the layout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dd0a438. Configure here.
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Adds a small Lisp for customizing the sidebar. A user drops a
render-rowfunction in
~/.config/cmux/sidebar.lisp; it receives one workspace's data andreturns a view tree that cmux renders as SwiftUI. With no script file present the
sidebar renders exactly as before, so this is fully opt-in and the default path
is unchanged.
New package
Packages/CmuxSidebarScript(standalone,swift test-able, no appdependency):
if/when/cond/let/def/fn/map/...) with:keywordview options so a form reads like SwiftUI.vstack/hstack/zstack/text/image/label/shapes/progress-view/button/...) and ~30 modifiers (:font :foreground :background :padding :frame :corner-radius :line-limit :truncation :shadow :overlay :on-tap ...). Adding a view or modifier is one registry entry plus one render case.Equatableview tree.RenderNodeViewturns it into SwiftUI. This split is the perf contract: a row recomputes its node only when its data changes and stays.equatable(), so untouched rows are skipped at 1000-workspace scale. It also makes evaluation fully unit-testable without booting SwiftUI.Integration in
TabItemView: when a script is active, the row's content rendersthrough
RenderNodeView; cmux keeps the selection background, close button,drag/drop, tap, and context menu. The script is passed in as an immutable value
(a plain
let, liketabManager) plus a version folded into==, so thesnapshot-boundary and typing-latency contracts hold. Compile/render faults log
and fall back to the native row.
Example
Testing
swift test --package-path Packages/CmuxSidebarScript— reader, evaluator, builtins, bridge, and end-to-end default-script rendering. All green../scripts/reload.sh --tag sidebar-lisp) builds and links intocmuxandcmux-unit.~/.config/cmux/sidebar.lisp, sidebar is unchanged; with the default script copied in, rows render through the engine.Notes / follow-ups
EquatableRenderNode already supports it) if profiling shows need at extreme scale.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Large new interpreter runs on the main thread during renders (bounded but still a perf/fault surface); user scripts drive UI actions and persisted sidebar state, though the host must wire handlers and the design avoids arbitrary code execution.
Overview
Introduces the
CmuxSidebarScriptSwift package: a Scheme-style Lisp that compiles user scripts into pureRenderNodetrees, thenRenderNodeViewmaps them to SwiftUI—without executing arbitrary Swift.Scripts can define
render-row(per-workspace row inside native chrome) orrender-sidebar(full sidebar). The evaluator enforces step/recursion/collection limits; top-level bindings freeze after compile. The bridge exposes stacks, shapes, ~30:keywordmodifiers, and actions (select-workspace,new-workspace,open-url, persistedset-sidebar-state/toggle-sidebar-state).SidebarScriptContext/SidebarScriptSidebarContextsupply workspace, file, and state records.Ships
DefaultSidebar.lispplus six full-sidebar demo presets andSidebarScriptDemofor discovery. Adds localized strings for Sidebar Layouts menu labels (native vs demo names).Reviewed by Cursor Bugbot for commit dd0a438. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a small Lisp engine to customize the sidebar, now supporting per-row
render-rowand fullrender-sidebarcontrol with safe fallback to the native UI. Includes layout presets and a titlebar Sidebar Layouts menu; the Finder preset now persists its tree state.New Features
CmuxSidebarScript: Reader, Evaluator (step budget, depth 64, generated-collection cap), user-facing errors (line numbers), and a SwiftUI bridge (views, actions likeopen-url/copy-text, ~30:keywordmodifiers).render-rowkeeps native row chrome;render-sidebarcan own the entire sidebar. PureRenderNode+RenderNodeViewkeep rows.equatable(); comprehensive tests.SidebarScriptStorecompiles once at sidebar construction; native fallback on compile/render errors. Binds truncation (tail/head/middle) and gradient direction (vertical/horizontal/diagonal) tokens. Titlebar/minimal-mode Sidebar Layouts menu to preview/install presets and manage the user script. ShipsDefaultSidebar.lispand distinct presets; the Finder preset persists expansion/selection state.Migration
~/.config/cmux/sidebar.lispwithrender-sidebarorrender-row.Written for commit dd0a438. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests