Repository navigation
Add editable reusable parameters to workspace layouts - #8085
Closed
austinywang wants to merge 20 commits into
Closed
austinywang wants to merge 20 commits into
austinywang wants to merge 20 commits into
Conversation
|
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 reusable workspace template variables with precedence-based resolution across workspace definitions, saved layouts, CLI commands, socket APIs, and launch flows. Repeatable ChangesWorkspace template parameterization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds launch-time
Confidence Score: 5/5Safe to merge — preflight resolution is atomic, backward compatibility is correctly gated, and the new socket contract is validated before reaching app state. The core template grammar, resolver, and workspace parameterization logic are well-structured and tested. Two minor code quality issues exist in CmuxConfigActionSaver: a dead guard-let branch that can never fire, and a compound guard that would surface a misleading error message if JSON parsing fails after config validation has already passed. WorkspaceTemplateErrorPresenter reuses the missing-parameters title key for its generic error branch. None of these affect current runtime correctness. Sources/CmuxConfigActionSaver.swift (dead guard and misleading error attribution) and Sources/WorkspaceTemplateErrorPresenter.swift (shared title key across both error branches). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Launch trigger"] --> B{"params present or explicit params?"}
B -- No --> C["Pass definition through unchanged"]
B -- Yes --> D["CmuxTemplateResolver.resolvedValues()"]
D --> E{"All variables resolved?"}
E -- No --> F["throw missingVariables"]
E -- Yes --> G["substitutingTemplateValues() atomically"]
G --> H["Create workspace"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Launch trigger"] --> B{"params present or explicit params?"}
B -- No --> C["Pass definition through unchanged"]
B -- Yes --> D["CmuxTemplateResolver.resolvedValues()"]
D --> E{"All variables resolved?"}
E -- No --> F["throw missingVariables"]
E -- Yes --> G["substitutingTemplateValues() atomically"]
G --> H["Create workspace"]
Reviews (9): Last reviewed commit: "Remove stale workspace prompt binding" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…zed-workspace-layouts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate+SavedLayoutMenu.swift (1)
54-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind and reuse the resolved window instead of discarding it.
The guard confirms
resolvedWindow(for: context)is non-nil but never captures it, so the catch block passespresentingWindow: niltoWorkspaceTemplateErrorPresenter. Per the presenter's own logic, a nil window forces a blockingNSAlert.runModal()instead of a sheet attached to the actual context-menu window — even though a valid window was just confirmed to exist.🪟 Proposed fix: bind and pass the resolved window
- guard let box = sender.representedObject as? SavedLayoutContextMenuActionBox, - let context = mainWindowContexts.values.first(where: { $0.windowId == box.windowId }), - resolvedWindow(for: context) != nil else { + guard let box = sender.representedObject as? SavedLayoutContextMenuActionBox, + let context = mainWindowContexts.values.first(where: { $0.windowId == box.windowId }), + let window = resolvedWindow(for: context) else { NSSound.beep() return } do { guard let layout = try SavedLayoutStore().layout(named: box.layoutName) else { NSSound.beep() return } _ = try context.tabManager.openWorkspace( fromSavedLayout: layout, cwdOverride: nil, focus: true ) } catch let error as CmuxTemplateResolutionError { - WorkspaceTemplateErrorPresenter(presentingWindow: nil).present(error) + WorkspaceTemplateErrorPresenter(presentingWindow: window).present(error) } catch { NSSound.beep() }🤖 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/AppDelegate`+SavedLayoutMenu.swift around lines 54 - 77, Update performSavedLayoutContextMenuItem to bind the non-nil result of resolvedWindow(for: context) in its guard, then pass that resolved window to WorkspaceTemplateErrorPresenter in the CmuxTemplateResolutionError catch instead of nil. Preserve the existing validation and error-handling behavior.
🤖 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 `@Resources/Localizable.xcstrings`:
- Line 43802: Update the layout help localization value for the cmux layout
usage text: remove the unsupported “export” subcommand from the description, and
document that layout open accepts bare --param KEY arguments alongside the
existing KEY=VALUE syntax. Keep the listed implemented subcommands and examples
aligned with the actual CLI contract.
In `@web/messages/ja.json`:
- Line 1815: Update the Japanese translation for parameterizedLayoutsCLI to
state that the value-less --param KEY form reads an exported environment
variable, replacing the current wording that refers to a shell variable while
preserving the rest of the message.
---
Outside diff comments:
In `@Sources/AppDelegate`+SavedLayoutMenu.swift:
- Around line 54-77: Update performSavedLayoutContextMenuItem to bind the
non-nil result of resolvedWindow(for: context) in its guard, then pass that
resolved window to WorkspaceTemplateErrorPresenter in the
CmuxTemplateResolutionError catch instead of nil. Preserve the existing
validation and error-handling behavior.
🪄 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: b357281d-19f8-4fbe-965d-818b32ac1cbd
📒 Files selected for processing (50)
CLI/CMUXCLI+WorkspaceTemplateParameters.swiftCLI/cmux.swiftCLI/cmux_layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlCommandCoordinator+Layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlLayoutContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorLayoutTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlLayoutContextTestStubs.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplate.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateResolutionError.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateResolver.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateVariable.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/CmuxTemplateTests.swiftResources/Localizable.xcstringsSources/AppDelegate+SavedLayoutMenu.swiftSources/CmuxConfigExecutor+WorkspaceParameterization.swiftSources/CmuxConfigExecutor.swiftSources/CmuxWorkspaceDefinition+Parameterization.swiftSources/CmuxWorkspaceDefinition.swiftSources/ContentView+SavedLayoutCommands.swiftSources/TabManager+SavedLayouts.swiftSources/TerminalController+ControlLayoutContext.swiftSources/TerminalController+WorkspaceCreate.swiftSources/TerminalController+WorkspaceTemplateParameters.swiftSources/WorkspaceTemplateErrorPresenter.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLIWorkspaceTemplateParameterTests.swiftcmuxTests/SavedLayoutDefinitionTests.swiftdocs/cli-contract.mdweb/app/[locale]/(landing)/docs/custom-commands/page.tsxweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
…-8059-parameterized-workspace-layouts # Conflicts: # cmux.xcodeproj/project.pbxproj
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)
CLI/cmux.swift (1)
7473-7473: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTranslate the updated CLI strings in every supported locale
cli.workspace.create.error.unknownFlag,cli.layout.help, andcli.layout.open.error.unknownFlagare only translated in a subset ofResources/Localizable.xcstringslocales. Add the missing entries for the remaining supported locales (ar,bs,da,de,es,fr,it,km,nb,pl,pt-BR,ru,th,tr,zh-Hans,zh-Hant) so these new messages don’t fall back to English.🤖 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 `@CLI/cmux.swift` at line 7473, Translate the updated CLI strings for cli.workspace.create.error.unknownFlag in CLI/cmux.swift:7473-7473, cli.layout.help in CLI/cmux_layout.swift:5-14, and cli.layout.open.error.unknownFlag in CLI/cmux_layout.swift:139-153 by adding entries for every missing supported locale: ar, bs, da, de, es, fr, it, km, nb, pl, pt-BR, ru, th, tr, zh-Hans, and zh-Hant. Update Resources/Localizable.xcstrings consistently so these keys have localized values instead of falling back to English.Source: Path instructions
🤖 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 `@CLI/cmux.swift`:
- Line 7473: Translate the updated CLI strings for
cli.workspace.create.error.unknownFlag in CLI/cmux.swift:7473-7473,
cli.layout.help in CLI/cmux_layout.swift:5-14, and
cli.layout.open.error.unknownFlag in CLI/cmux_layout.swift:139-153 by adding
entries for every missing supported locale: ar, bs, da, de, es, fr, it, km, nb,
pl, pt-BR, ru, th, tr, zh-Hans, and zh-Hant. Update
Resources/Localizable.xcstrings consistently so these keys have localized values
instead of falling back to English.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94ade4d8-afd7-4596-b58c-31d2790e9d3d
📒 Files selected for processing (2)
CLI/cmux.swiftCLI/cmux_layout.swift
…-8059-parameterized-workspace-layouts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CmuxWorkspaceDefinition+Parameterization.swift (1)
79-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer sorting dictionary elements directly.
Iterating over the dictionary's keys to perform a redundant optional lookup
env?[key]is unidiomatic. You can directly sort the dictionary's elements and map the values, which is cleaner and avoids the lookup.♻️ Proposed refactor
- templates.append(contentsOf: (env ?? [:]).keys.sorted().compactMap { key in - env?[key].map(CmuxTemplate.init) - }) + templates.append(contentsOf: (env ?? [:]).sorted(by: { $0.key < $1.key }).map { + CmuxTemplate($0.value) + })🤖 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/CmuxWorkspaceDefinition`+Parameterization.swift around lines 79 - 81, Update the template appending logic to sort the dictionary elements from env directly, then map each element’s value into CmuxTemplate without iterating keys or performing optional lookups. Preserve the existing empty/nil environment behavior.
🤖 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 `@CLI/cmux.swift`:
- Around line 7482-7485: Update the parameter construction around cwdOpt so
params["caller_cwd"] is always set to FileManager.default.currentDirectoryPath,
while params["cwd"] remains conditional on an explicitly provided cwdOpt.
In `@Sources/TerminalController`+WorkspaceCreate.swift:
- Around line 139-156: The template resolution flow around
resolver.resolvedValues must run regardless of whether templateParameters is
empty. Remove the templateParameters.isEmpty() bypass, always resolve and
substitute templateDefinition, initialCommandTemplate, initialEnvTemplate, and
descriptionTemplate, while preserving workspaceTemplateResolutionFailure(error)
handling for resolution failures.
In `@web/messages/fr.json`:
- Around line 420-425: Update the French localization values for summary and
destinationsTitle to use consistent fork terminology such as “Dupliquez” or
“Forkez” instead of “Branchez”, while preserving the surrounding meaning and all
other translations unchanged.
---
Outside diff comments:
In `@Sources/CmuxWorkspaceDefinition`+Parameterization.swift:
- Around line 79-81: Update the template appending logic to sort the dictionary
elements from env directly, then map each element’s value into CmuxTemplate
without iterating keys or performing optional lookups. Preserve the existing
empty/nil environment behavior.
🪄 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: 872bc6a1-2dca-4b2e-bb28-7de4265380c9
📒 Files selected for processing (46)
CLI/CMUXCLI+WorkspaceTemplateParameters.swiftCLI/cmux.swiftCLI/cmux_layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlCommandCoordinator+Layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlLayoutContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorLayoutTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlLayoutContextTestStubs.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateParameterInput.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/CmuxTemplateTests.swiftResources/Localizable.xcstringsSources/AppDelegate+SavedLayoutMenu.swiftSources/CmuxConfigExecutor+WorkspaceParameterization.swiftSources/CmuxConfigExecutor.swiftSources/CmuxWorkspaceDefinition+Parameterization.swiftSources/ContentView+SavedLayoutCommands.swiftSources/TabManager+SavedLayouts.swiftSources/TerminalController+ControlLayoutContext.swiftSources/TerminalController+WorkspaceCreate.swiftSources/WorkspaceTemplateParameterPrompt.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLIWorkspaceTemplateParameterTests.swiftcmuxTests/SavedLayoutDefinitionTests.swiftcmuxTests/WorkspaceTemplateParameterPromptTests.swiftdocs/cli-contract.mdweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
💤 Files with no reviewable changes (1)
- CLI/CMUXCLI+WorkspaceTemplateParameters.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CmuxWorkspaceDefinition+Parameterization.swift (1)
79-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer sorting dictionary elements directly.
Iterating over the dictionary's keys to perform a redundant optional lookup
env?[key]is unidiomatic. You can directly sort the dictionary's elements and map the values, which is cleaner and avoids the lookup.♻️ Proposed refactor
- templates.append(contentsOf: (env ?? [:]).keys.sorted().compactMap { key in - env?[key].map(CmuxTemplate.init) - }) + templates.append(contentsOf: (env ?? [:]).sorted(by: { $0.key < $1.key }).map { + CmuxTemplate($0.value) + })🤖 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/CmuxWorkspaceDefinition`+Parameterization.swift around lines 79 - 81, Update the template appending logic to sort the dictionary elements from env directly, then map each element’s value into CmuxTemplate without iterating keys or performing optional lookups. Preserve the existing empty/nil environment behavior.
🤖 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 `@CLI/cmux.swift`:
- Around line 7482-7485: Update the parameter construction around cwdOpt so
params["caller_cwd"] is always set to FileManager.default.currentDirectoryPath,
while params["cwd"] remains conditional on an explicitly provided cwdOpt.
In `@Sources/TerminalController`+WorkspaceCreate.swift:
- Around line 139-156: The template resolution flow around
resolver.resolvedValues must run regardless of whether templateParameters is
empty. Remove the templateParameters.isEmpty() bypass, always resolve and
substitute templateDefinition, initialCommandTemplate, initialEnvTemplate, and
descriptionTemplate, while preserving workspaceTemplateResolutionFailure(error)
handling for resolution failures.
In `@web/messages/fr.json`:
- Around line 420-425: Update the French localization values for summary and
destinationsTitle to use consistent fork terminology such as “Dupliquez” or
“Forkez” instead of “Branchez”, while preserving the surrounding meaning and all
other translations unchanged.
---
Outside diff comments:
In `@Sources/CmuxWorkspaceDefinition`+Parameterization.swift:
- Around line 79-81: Update the template appending logic to sort the dictionary
elements from env directly, then map each element’s value into CmuxTemplate
without iterating keys or performing optional lookups. Preserve the existing
empty/nil environment behavior.
🪄 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: 872bc6a1-2dca-4b2e-bb28-7de4265380c9
📒 Files selected for processing (46)
CLI/CMUXCLI+WorkspaceTemplateParameters.swiftCLI/cmux.swiftCLI/cmux_layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlCommandCoordinator+Layout.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Layout/ControlLayoutContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorLayoutTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlLayoutContextTestStubs.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateParameterInput.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplateResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/CmuxTemplateTests.swiftResources/Localizable.xcstringsSources/AppDelegate+SavedLayoutMenu.swiftSources/CmuxConfigExecutor+WorkspaceParameterization.swiftSources/CmuxConfigExecutor.swiftSources/CmuxWorkspaceDefinition+Parameterization.swiftSources/ContentView+SavedLayoutCommands.swiftSources/TabManager+SavedLayouts.swiftSources/TerminalController+ControlLayoutContext.swiftSources/TerminalController+WorkspaceCreate.swiftSources/WorkspaceTemplateParameterPrompt.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLIWorkspaceTemplateParameterTests.swiftcmuxTests/SavedLayoutDefinitionTests.swiftcmuxTests/WorkspaceTemplateParameterPromptTests.swiftdocs/cli-contract.mdweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
💤 Files with no reviewable changes (1)
- CLI/CMUXCLI+WorkspaceTemplateParameters.swift
🛑 Comments failed to post (3)
CLI/cmux.swift (1)
7482-7485: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Send
caller_cwdunconditionally for accurate template path resolution.When
--cwdis omitted but a layout or default template parameter specifies a relative path for the working directory, the server relies oncaller_cwdto resolve it against the CLI's invocation directory. By scopingcaller_cwdinside theif let cwdOptblock, omitted--cwdflags will cause relative paths to silently resolve against the server's fallback directory (usually~) instead of the current working directory.💡 Proposed fix
+ params["caller_cwd"] = FileManager.default.currentDirectoryPath if let cwdOpt { params["cwd"] = cwdOpt - params["caller_cwd"] = FileManager.default.currentDirectoryPath }📝 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.params["caller_cwd"] = FileManager.default.currentDirectoryPath if let cwdOpt { params["cwd"] = cwdOpt }🤖 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 `@CLI/cmux.swift` around lines 7482 - 7485, Update the parameter construction around cwdOpt so params["caller_cwd"] is always set to FileManager.default.currentDirectoryPath, while params["cwd"] remains conditional on an explicitly provided cwdOpt.Sources/TerminalController+WorkspaceCreate.swift (1)
139-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not skip resolution when explicit parameters are empty.
This branch leaves process-environment and inline-default placeholders literal, and lets missing required values reach workspace creation. Always preflight and substitute the templates; explicit parameters are only the highest-precedence source, not the feature gate.
Proposed fix
- if templateParameters.isEmpty { - resolvedDefinition = templateDefinition - initialCommand = initialCommandTemplate - initialEnv = initialEnvTemplate - description = descriptionTemplate - } else { - do { - let values = try resolver.resolvedValues( - for: templateDefinition.templateStrings + additionalTemplates - ) - resolvedDefinition = templateDefinition.substitutingTemplateValues(values) - initialCommand = initialCommandTemplate.map { CmuxTemplate($0).substituting(values) } - initialEnv = initialEnvTemplate.mapValues { CmuxTemplate($0).substituting(values) } - description = descriptionTemplate.map { CmuxTemplate($0).substituting(values) } - } catch { - return workspaceTemplateResolutionFailure(error) - } + do { + let values = try resolver.resolvedValues( + for: templateDefinition.templateStrings + additionalTemplates + ) + resolvedDefinition = templateDefinition.substitutingTemplateValues(values) + initialCommand = initialCommandTemplate.map { CmuxTemplate($0).substituting(values) } + initialEnv = initialEnvTemplate.mapValues { CmuxTemplate($0).substituting(values) } + description = descriptionTemplate.map { CmuxTemplate($0).substituting(values) } + } catch { + return workspaceTemplateResolutionFailure(error) }📝 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.do { let values = try resolver.resolvedValues( for: templateDefinition.templateStrings + additionalTemplates ) resolvedDefinition = templateDefinition.substitutingTemplateValues(values) initialCommand = initialCommandTemplate.map { CmuxTemplate($0).substituting(values) } initialEnv = initialEnvTemplate.mapValues { CmuxTemplate($0).substituting(values) } description = descriptionTemplate.map { CmuxTemplate($0).substituting(values) } } catch { return workspaceTemplateResolutionFailure(error) }🤖 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/TerminalController`+WorkspaceCreate.swift around lines 139 - 156, The template resolution flow around resolver.resolvedValues must run regardless of whether templateParameters is empty. Remove the templateParameters.isEmpty() bypass, always resolve and substitute templateDefinition, initialCommandTemplate, initialEnvTemplate, and descriptionTemplate, while preserving workspaceTemplateResolutionFailure(error) handling for resolution failures.web/messages/fr.json (1)
420-425: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use French fork terminology instead of “Branchez”.
“Branchez une conversation” means connecting or plugging it in, not creating a conversation fork. Use a consistent term such as “Dupliquez” or “Forkez” in the summary and destination heading.
🤖 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 `@web/messages/fr.json` around lines 420 - 425, Update the French localization values for summary and destinationsTitle to use consistent fork terminology such as “Dupliquez” or “Forkez” instead of “Branchez”, while preserving the surrounding meaning and all other translations unchanged.
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)
Sources/TerminalController+WorkspaceCreate.swift (1)
137-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlways resolve templates to support environment variables, inline defaults, and catch missing variables consistently.
The check
if templateParameters.isEmptybypasses template resolution entirely when no explicit parameters are passed in the request. This prevents process environment variables (e.g.,{{HOME}}) and inline defaults (e.g.,{{port=8080}}) from resolving, and causes inconsistent validation behavior where missing placeholders are silently ignored if no parameters are passed.To fix this, check if the templates actually contain variables instead of relying on the
templateParameterspayload size.🐛 Proposed fix
let resolvedDefinition: CmuxWorkspaceDefinition let initialCommand: String? let initialInput: String? let initialEnv: [String: String] let description: String? - if templateParameters.isEmpty { + let allTemplates = templateDefinition.templateStrings + additionalTemplates + + if !allTemplates.contains(where: \.containsVariables) { resolvedDefinition = templateDefinition initialCommand = initialCommandTemplate initialInput = initialInputTemplate initialEnv = initialEnvTemplate description = descriptionTemplate } else { do { - let values = try resolver.resolvedValues( - for: templateDefinition.templateStrings + additionalTemplates - ) + let values = try resolver.resolvedValues(for: allTemplates) resolvedDefinition = templateDefinition.substitutingTemplateValues(values) initialCommand = initialCommandTemplate.map { CmuxTemplate($0).substituting(values) } initialInput = initialInputTemplate.map { CmuxTemplate($0).substituting(values) } initialEnv = initialEnvTemplate.mapValues { CmuxTemplate($0).substituting(values) } description = descriptionTemplate.map { CmuxTemplate($0).substituting(values) } } catch { return workspaceTemplateResolutionFailure(error) } }🤖 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/TerminalController`+WorkspaceCreate.swift around lines 137 - 161, Update the branching around template resolution to determine whether the combined templates contain variables, rather than checking templateParameters.isEmpty. Always invoke resolver.resolvedValues when templateDefinition, command, input, environment, or description templates contain placeholders, including environment variables and inline defaults; retain the direct-value path only when no templates require resolution, and preserve workspaceTemplateResolutionFailure(error) handling.
🤖 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 `@Sources/TerminalController`+WorkspaceCreate.swift:
- Around line 137-161: Update the branching around template resolution to
determine whether the combined templates contain variables, rather than checking
templateParameters.isEmpty. Always invoke resolver.resolvedValues when
templateDefinition, command, input, environment, or description templates
contain placeholders, including environment variables and inline defaults;
retain the direct-value path only when no templates require resolution, and
preserve workspaceTemplateResolutionFailure(error) handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0027434c-6fc9-4940-83b6-2635d4cdbbb0
📒 Files selected for processing (5)
CLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/ConfigValues/Template/CmuxTemplate.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/CmuxTemplateTests.swiftSources/TerminalController+WorkspaceCreate.swiftcmuxTests/CLIWorkspaceTemplateParameterTests.swift
austinywang
force-pushed
the
issue-8059-parameterized-workspace-layouts
branch
3 times, most recently
from
July 15, 2026 23:50
b976f19 to
01d22d1
Compare
austinywang
force-pushed
the
issue-8059-parameterized-workspace-layouts
branch
from
July 16, 2026 00:12
01d22d1 to
e8fe3b3
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
austinywang
changed the title
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8059
Related: #6557, #6898
Summary
{{name}}and{{name=default}}substitution across workspace and surface names, paths, environment values, setup commands, terminal commands, and browser URLsparams, whose saved values are reused automatically every time the layout opens--paramand v2template_params, including atomic missing-value validation before workspace mutationUX and ownership
paramsis the reusable source of truth. Selecting a configured layout from the plus menu, Command Palette, surface action, or saved-layout menu resolves and launches it immediately. Editing happens only when the user chooses the explicit Manage Layouts editor, where fields are prefilled and Save Changes atomically updatesactions.<id>.workspace.paramsin the globalcmux.jsonwhile preserving JSONC comments and surrounding formatting.Project-local actions,
workspaceCommanddefinitions, and~/.config/cmux/layouts.jsonentries remain source-owned and are edited in their configuration files. Missing required values abort the whole launch and explain where to edit them.Definitions without
paramsretain literal{{...}}text for backward compatibility.Resolution model
Precedence, highest first:
--param/template_paramsparamsenvvaluesValues are non-recursive. Workspace definitions substitute literal values because a parameter may represent a path, URL, title, environment value, or command fragment.
Validation
e8fe3b3ea2: 15 tests ran, with the new no-prompt test failing because the old code displayed a sheet and created no workspaceissue-8059-explicit-editor-green-pushed-16bff0fa177b: 60/60 tests passed across action execution, parameter persistence, menu rendering, editor sizing, and saved layouts341a9e752d: 15/15CmuxConfigWorkspaceActionTestspassed in runissue-8059-explicit-editor-green-pushed-34c414ff84caios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolved, an upstream file added by currentmain; this PR does not modify that fileLocalization was audited across the app string catalog and all 20 web catalogs. Every changed key is present in every supported locale, and rich-text tags/placeholders match English.