Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/swift-file-length-budget.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
3926 cmuxTests/TabManagerUnitTests.swift
3903 cmuxTests/WindowAndDragTests.swift
3699 cmuxTests/CLIGenericHookPersistenceTests.swift
3672 Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
3734 Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift
3397 Sources/CmuxConfig.swift
3331 cmuxTests/TabManagerSessionSnapshotTests.swift
3055 Sources/Update/UpdateTitlebarAccessory.swift
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
/// Appended at the end so existing persisted raw values (user accessory bar
/// order/enabled set) are preserved.
case composer
/// Send a carriage return (Enter). Appended at the end so existing persisted
/// raw values (which are the `Int` rawValues, stored as `builtin.<n>`) stay
/// stable; its default on-bar position is curated separately in
/// ``defaultConfigurableOrder``.
case returnKey
var title: String {
title(isMacRemote: false)
}
Expand All @@ -234,6 +239,8 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
return String(localized: "terminal.input_accessory.title.escape", defaultValue: "Esc")
case .tab:
return String(localized: "terminal.input_accessory.title.tab", defaultValue: "Tab")
case .returnKey:
return "⏎"
case .ctrlC:
return "^C"
case .ctrlD:
Expand Down Expand Up @@ -288,6 +295,7 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
case .composer: return "terminal.inputAccessory.composer"
case .escape: return "terminal.inputAccessory.escape"
case .tab: return "terminal.inputAccessory.tab"
case .returnKey: return "terminal.inputAccessory.return"
case .upArrow: return "terminal.inputAccessory.up"
case .downArrow: return "terminal.inputAccessory.down"
case .leftArrow: return "terminal.inputAccessory.left"
Expand Down Expand Up @@ -360,14 +368,16 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
}
}

var output: Data? {
public var output: Data? {
switch self {
case .control, .alternate, .command, .shift, .zoomOut, .zoomIn, .paste, .composer:
return nil
case .escape:
return Data([0x1B])
case .tab:
return Data([0x09])
case .returnKey:
return Data([0x0D]) // CR (Enter)
case .tilde:
return Data([0x7E]) // ~
case .pipe:
Expand Down Expand Up @@ -450,10 +460,10 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {

/// The default on-bar arrangement of the configurable shortcuts: the leading
/// modifier/paste controls, then the high-traffic agent and control keys (Tab,
/// Esc, ^C/^D, the Claude/Codex launchers, the arrow keys, Clear), then the
/// punctuation and navigation keys, then the trailing zoom controls. Esc sits
/// immediately to the right of Tab so the two most common terminal keys are
/// adjacent. Curated independently of the enum's `rawValue` order so the
/// Esc, Return, ^C/^D, the Claude/Codex launchers, the arrow keys, Clear), then
/// the punctuation and navigation keys, then the trailing zoom controls. Esc and
/// Return sit immediately to the right of Tab so the most common terminal keys
/// are adjacent. Curated independently of the enum's `rawValue` order so the
/// default bar can be arranged without perturbing the persisted identifiers,
/// which are the `rawValue`s.
///
Expand All @@ -464,6 +474,7 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
defaultLeadingActions + [
.tab,
.escape,
.returnKey,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enable Return during the v3 upgrade path

For users who have already launched a v3 toolbar config, adding .returnKey to defaultConfigurableOrder only appends it to displayOrder; TerminalAccessoryConfiguration.init loads the existing non-nil cmux.terminal.toolbar.enabled.v3 set, and TerminalAccessoryLayoutReducer.load keeps that set unchanged, so the new Return button is hidden after upgrade unless the user resets or manually enables it. If this shortcut is meant to be added to the terminal bar for existing users, the v3 migration/load path needs to force-enable this new ID or bump the persisted schema.

Useful? React with 👍 / 👎.

.ctrlC, .ctrlD,
.claude, .codex,
.upArrow, .downArrow, .leftArrow, .rightArrow,
Expand All @@ -480,6 +491,7 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable {
switch self {
case .escape: return String(localized: "terminal.shortcut.name.escape", defaultValue: "Escape")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Missing xcstrings entry for terminal.shortcut.name.return

settingsDisplayName calls String(localized: "terminal.shortcut.name.return", defaultValue: "Return"), but ios/cmux/Resources/Localizable.xcstrings has no entry for this key — unlike every other nearby shortcut name (.escape → terminal.shortcut.name.escape, .tab → terminal.shortcut.name.tab, etc., each with en + ja translations). On a Japanese-locale device, users will see the raw English word "Return" in the settings display because the runtime falls back to defaultValue. Add "terminal.shortcut.name.return" to the catalog with at least en and ja entries.

Rule Used: Flag production user-facing text that is not fully... (source)

case .tab: return String(localized: "terminal.shortcut.name.tab", defaultValue: "Tab")
case .returnKey: return String(localized: "terminal.shortcut.name.return", defaultValue: "Return")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Add catalog entries for the Return label

This introduces the new user-facing key terminal.shortcut.name.return, but I could not find it in ios/cmux/Resources/Localizable.xcstrings (the sibling shortcut keys there have en and ja entries). In Japanese settings, the shortcuts editor will fall back to the English default value, which violates the repo's requirement that every user-facing string be localized in the catalog for all supported locales.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add the matching string-catalog entry for terminal.shortcut.name.return.

Line 496 introduces a new user-facing localization key, but this review cohort does not include the corresponding Resources/Localizable.xcstrings entry. That leaves the shortcuts editor falling back to English "Return" anywhere the catalog is missing instead of shipping a translated label for every supported locale. As per coding guidelines, "Flag production changes that introduce new Swift localization keys not backed by matching Resources/*.xcstrings entries with translated values for every supported locale" and "All user-facing strings must be localized."

🤖 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/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`
at line 496, The localization key `terminal.shortcut.name.return` introduced in
the case statement for `.returnKey` is missing its corresponding entry in the
string catalog. Add an entry for this key to the Resources/Localizable.xcstrings
file with the value "Return" and provide translations for all supported locales
that your application targets. Without this catalog entry, users in non-English
locales will see the default English fallback text instead of proper
translations for the shortcuts editor label.

Source: Coding guidelines

case .upArrow: return String(localized: "terminal.shortcut.name.upArrow", defaultValue: "Up Arrow")
case .downArrow: return String(localized: "terminal.shortcut.name.downArrow", defaultValue: "Down Arrow")
case .leftArrow: return String(localized: "terminal.shortcut.name.leftArrow", defaultValue: "Left Arrow")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,29 +92,17 @@ public final class TerminalAccessoryConfiguration {
let order = v3Order.compactMap(ToolbarItemID.init(storageKey:))
let enabled = (defaults.array(forKey: Self.enabledDefaultsKey) as? [String])?
.compactMap(ToolbarItemID.init(storageKey:))
// ⇧ became user-configurable after the v3 schema shipped, so a layout
// persisted under the v3 keys has no record of it and a naive load
// would leave it hidden. Fold ⇧ in next to the other modifiers and
// show it — once, keyed off ⇧'s absence from the saved order — so an
// existing install surfaces it like a fresh one. Once ⇧ is persisted
// into the order it is present on every later launch, so the fold
// becomes a no-op and a user who then hides ⇧ keeps it hidden.
if let folded = migration.foldingNewlyConfigurable(
TerminalInputAccessoryAction.shift.itemID,
after: [
TerminalInputAccessoryAction.command.itemID,
TerminalInputAccessoryAction.alternate.itemID,
TerminalInputAccessoryAction.control.itemID,
],
order: order,
enabled: enabled ?? order
) {
savedOrder = folded.order
savedEnabled = folded.enabled
} else {
savedOrder = order
savedEnabled = enabled
}
// ⇧ and Return each became user-configurable after the v3 schema
// shipped, so a layout persisted under the v3 keys has no record of
// them and a naive load would leave them hidden. Fold each in next to
// its neighbours and show it — once per id, keyed off that id's
// absence from the saved order — so an existing install surfaces them
// like a fresh one. Once an id is persisted into the order it is
// present on every later launch, so the fold becomes a no-op and a
// user who then hides it keeps it hidden.
let folded = Self.foldNewlyConfigurableV3(order: order, enabled: enabled, migration: migration)
savedOrder = folded.order
savedEnabled = folded.enabled
} else if let v2Order = defaults.array(forKey: Self.legacyV2OrderDefaultsKey) as? [String] {
let widened = migration.widenedToV3(
order: v2Order.compactMap(ToolbarItemID.init(storageKey:)),
Expand All @@ -123,8 +111,11 @@ public final class TerminalAccessoryConfiguration {
forcedLeading: Self.forcedLeadingRawValues,
forcedTrailing: Self.forcedTrailingRawValues
)
savedOrder = widened.order
savedEnabled = widened.enabled
// The widening force-enables the pinned modifiers/zoom but never the
// post-v3 shortcuts; a v2 config predates Return, so fold it in too.
let folded = Self.foldNewlyConfigurableV3(order: widened.order, enabled: widened.enabled, migration: migration)
savedOrder = folded.order
savedEnabled = folded.enabled
} else if let v1Order = defaults.array(forKey: Self.legacyV1OrderDefaultsKey) as? [Int] {
let widened = migration.widenedToV3(
order: migration.migratedOrder(legacy: v1Order),
Expand All @@ -134,8 +125,11 @@ public final class TerminalAccessoryConfiguration {
forcedLeading: Self.forcedLeadingRawValues,
forcedTrailing: Self.forcedTrailingRawValues
)
savedOrder = widened.order
savedEnabled = widened.enabled
// Same as the v2 path: fold in the post-v3 shortcuts the widening
// doesn't cover (Return), force-shown, exactly once.
let folded = Self.foldNewlyConfigurableV3(order: widened.order, enabled: widened.enabled, migration: migration)
savedOrder = folded.order
savedEnabled = folded.enabled
} else {
savedOrder = []
savedEnabled = nil
Expand All @@ -149,6 +143,51 @@ public final class TerminalAccessoryConfiguration {
persist()
}

/// Folds the built-ins that became user-configurable *after* the v3 schema
/// shipped (⇧, then Return) into a persisted v3 layout, each adjacent to its
/// canonical neighbours and force-shown, exactly once per id. Returns the
/// possibly-folded order and enabled set; `enabled` is passed through
/// unchanged (including `nil`) when no fold applies, so a config already
/// carrying both keeps its authoritative shown/hidden choices.
private static func foldNewlyConfigurableV3(
order: [ToolbarItemID],
enabled: [ToolbarItemID]?,
migration: ToolbarLayoutMigration
) -> (order: [ToolbarItemID], enabled: [ToolbarItemID]?) {
var resolvedOrder = order
var resolvedEnabled = enabled
// Each entry is the newly-configurable id plus the predecessors to insert
// it after (first present wins; front if none present), mirroring
// `defaultConfigurableOrder`: ⇧ trails the other modifiers, Return trails
// Esc (then Tab, then the modifiers).
let folds: [(id: ToolbarItemID, anchors: [ToolbarItemID])] = [
(TerminalInputAccessoryAction.shift.itemID, [
TerminalInputAccessoryAction.command.itemID,
TerminalInputAccessoryAction.alternate.itemID,
TerminalInputAccessoryAction.control.itemID,
]),
(TerminalInputAccessoryAction.returnKey.itemID, [
TerminalInputAccessoryAction.escape.itemID,
TerminalInputAccessoryAction.tab.itemID,
TerminalInputAccessoryAction.command.itemID,
TerminalInputAccessoryAction.alternate.itemID,
TerminalInputAccessoryAction.control.itemID,
]),
]
for fold in folds {
if let widened = migration.foldingNewlyConfigurable(
fold.id,
after: fold.anchors,
order: resolvedOrder,
enabled: resolvedEnabled ?? resolvedOrder
) {
resolvedOrder = widened.order
resolvedEnabled = widened.enabled
}
}
return (resolvedOrder, resolvedEnabled)
}

/// `rawValue`s of the built-ins that v1/v2 pinned at the front of the bar,
/// passed to the v3 widening migration so they are force-enabled and inserted
/// at the front for an upgrading user.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,57 @@ struct ToolbarLayoutMigrationFoldTests {
let commandIndex = try #require(layout.order.firstIndex(of: .builtin(2)))
#expect(layout.order[commandIndex + 1] == shift)
}

// Two ids that each landed after the v3 schema shipped (⇧, then a later
// shortcut standing in for Return) must each fold in exactly once when the
// config layer chains the folds. escape=6 is the Return anchor.
private let returnKey = ToolbarItemID.builtin(30)
private let returnAnchors = [6, 7, 2, 1, 0].map { ToolbarItemID.builtin($0) } // escape, tab, then modifiers

@Test("chaining two folds inserts each newly-configurable id after its own anchor")
func chainsTwoFoldsIndependently() throws {
// A pre-⇧, pre-Return v3 order: modifiers, paste(27), tab(7), escape(6).
let order = [0, 1, 2, 27, 7, 6].map { ToolbarItemID.builtin($0) }
let afterShift = try #require(migration.foldingNewlyConfigurable(
shift, after: anchors, order: order, enabled: order
))
let afterReturn = try #require(migration.foldingNewlyConfigurable(
returnKey, after: returnAnchors, order: afterShift.order, enabled: afterShift.enabled
))
// ⇧ sits right after command(2); Return right after escape(6).
let commandIndex = try #require(afterReturn.order.firstIndex(of: .builtin(2)))
#expect(afterReturn.order[commandIndex + 1] == shift)
let escIndex = try #require(afterReturn.order.firstIndex(of: .builtin(6)))
#expect(afterReturn.order[escIndex + 1] == returnKey)
#expect(afterReturn.enabled.contains(shift))
#expect(afterReturn.enabled.contains(returnKey))
}

@Test("re-running the chained folds is a no-op once both ids are present")
func chainedFoldsAreIdempotent() throws {
let order = [0, 1, 2, 27, 7, 6].map { ToolbarItemID.builtin($0) }
let afterShift = try #require(migration.foldingNewlyConfigurable(
shift, after: anchors, order: order, enabled: order
))
let afterReturn = try #require(migration.foldingNewlyConfigurable(
returnKey, after: returnAnchors, order: afterShift.order, enabled: afterShift.enabled
))
// A second pass over the already-folded layout adds nothing.
#expect(migration.foldingNewlyConfigurable(
shift, after: anchors, order: afterReturn.order, enabled: afterReturn.enabled
) == nil)
#expect(migration.foldingNewlyConfigurable(
returnKey, after: returnAnchors, order: afterReturn.order, enabled: afterReturn.enabled
) == nil)
}

@Test("a user who hid the newly-configurable id keeps it hidden and unmoved")
func foldRespectsAnExistingHiddenId() {
// Return already in the order but hidden — the fold must not re-show it.
let order = [0, 1, 2, 6, 30].map { ToolbarItemID.builtin($0) }
let enabled = [0, 1, 2, 6].map { ToolbarItemID.builtin($0) }
#expect(migration.foldingNewlyConfigurable(
returnKey, after: returnAnchors, order: order, enabled: enabled
) == nil)
}
}
17 changes: 17 additions & 0 deletions ios/cmux/Resources/Localizable.xcstrings
Original file line number Diff line number Diff line change
Expand Up @@ -5577,6 +5577,23 @@
}
}
},
"terminal.shortcut.name.return": {
"extractionState": "manual",
"localizations": {
"en": {
"stringUnit": {
"state": "translated",
"value": "Return"
}
},
"ja": {
"stringUnit": {
"state": "translated",
"value": "リターン"
}
}
}
},
"terminal.shortcut.name.tab": {
"extractionState": "manual",
"localizations": {
Expand Down
Loading
Loading