Repository navigation
iOS: add Return shortcut to terminal accessory bar - #6101
Conversation
Adds a Return/Enter key alongside Tab and Esc on the iOS terminal keyboard accessory bar. It sends a carriage return (CR, 0x0D), which is what Enter sends in a terminal. The new case is appended last in TerminalInputAccessoryAction so existing persisted raw values stay stable (the persisted key is builtin.<rawValue>). Its default on-bar position is curated in defaultConfigurableOrder, placed immediately after Esc so Tab/Esc/Return are adjacent. Handles every exhaustive switch over the enum: title, accessibilityIdentifier, output (CR byte), and settingsDisplayName. Dispatch flows through the generic action.output path (no perform-switch change needed). isUserConfigurable returns true by default so it appears in the bar and editor. Tests live at the ios/cmuxPackage layer (the enum is iOS-only, imports UIKit, so it is not reachable from host swift-test): assert Return is shown by default adjacent to Esc, sends CR, and that its storage key is stable. Host-testable reducer tests in CmuxMobileTerminalKit still pass. 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 a new ChangesreturnKey Action with Generalized Migration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
![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: def28d1211
ℹ️ 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".
| defaultLeadingActions + [ | ||
| .tab, | ||
| .escape, | ||
| .returnKey, |
There was a problem hiding this comment.
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 👍 / 👎.
| switch self { | ||
| case .escape: return String(localized: "terminal.shortcut.name.escape", defaultValue: "Escape") | ||
| case .tab: return String(localized: "terminal.shortcut.name.tab", defaultValue: "Tab") | ||
| case .returnKey: return String(localized: "terminal.shortcut.name.return", defaultValue: "Return") |
There was a problem hiding this comment.
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 👍 / 👎.
Greptile SummaryAdds a Return/Enter shortcut (
Confidence Score: 5/5Safe to merge — the change is confined to the iOS terminal accessory bar, uses the existing generic output dispatch path, and the one-time migration fold is idempotent and well-tested across all upgrade paths. The enum case is appended last so no existing stored raw values shift. The migration fold correctly handles nil enabled sets, chains with the prior ⇧ fold, and re-persists under v3 keys so it runs once. The ⏎ symbol title is consistent with the established pattern for non-text symbols, and settingsDisplayName is fully localized with en + ja catalog entries. Test coverage is thorough: default ordering, CR output, raw-value stability, v3/v2 migration, idempotence, and user-hide persistence are all exercised. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TerminalAccessoryConfiguration init] --> B{v3 keys present?}
B -- yes --> C[Load order + enabled from UserDefaults]
B -- no --> D{v2 keys present?}
D -- yes --> E[widenedToV3]
D -- no --> F{v1 keys present?}
F -- yes --> G[migratedOrder → widenedToV3]
F -- no --> H[Fresh install: empty order, nil enabled]
C --> I[foldNewlyConfigurableV3]
E --> I
G --> I
I --> J{⇧ absent from order?}
J -- yes --> K[Insert ⇧ after command/alt/ctrl, force-enable]
J -- no --> L[No-op for ⇧]
K --> M{Return absent from order?}
L --> M
M -- yes --> N[Insert Return after Esc/Tab/modifiers, force-enable]
M -- no --> O[No-op for Return]
N --> P[Persist under v3 keys]
O --> P
H --> P
P --> Q[reducer.load → displayOrder + enabledSet]
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -482,6 +493,7 @@ public enum TerminalInputAccessoryAction: Int, CaseIterable, Sendable { | |||
| switch self { | |||
| case .escape: return String(localized: "terminal.shortcut.name.escape", defaultValue: "Escape") | |||
There was a problem hiding this comment.
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)
![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
🤖 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
`@ios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift`:
- Around line 71-83: In the returnKeyStableIdentifier() test function, replace
the dynamic string construction that rebuilds the expected value from the same
enum case with a hardcoded literal string (such as "builtin.29") to ensure the
test catches any unintended changes to the persisted identifier. Additionally,
remove the maxRaw assertion and related lines that only verify the current
ordering rather than the stability of the stored key itself. This ensures the
test will fail if enum reordering accidentally changes the persisted identifier.
In
`@Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift`:
- 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.
🪄 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: 7e2c5930-35f9-4417-85d0-6e8b9882a89b
📒 Files selected for processing (2)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift
| @Test("Return's persisted identifier is stable") | ||
| func returnKeyStableIdentifier() { | ||
| // The persisted key is `builtin.<rawValue>`; Return is appended last in the | ||
| // enum so existing built-ins keep their raw values. Lock the storage key so | ||
| // a future reorder of the enum cannot silently shift it. | ||
| let stored = TerminalInputAccessoryAction.returnKey.itemID.storageKey | ||
| #expect(stored == "builtin.\(TerminalInputAccessoryAction.returnKey.rawValue)") | ||
| let parsed = ToolbarItemID(storageKey: stored) | ||
| #expect(parsed == id(.returnKey)) | ||
| // Appended last: its raw value is the max across all cases. | ||
| let maxRaw = TerminalInputAccessoryAction.allCases.map(\.rawValue).max() | ||
| #expect(TerminalInputAccessoryAction.returnKey.rawValue == maxRaw) | ||
| } |
There was a problem hiding this comment.
Pin the concrete persisted ID instead of recomputing it.
stored == "builtin.\(TerminalInputAccessoryAction.returnKey.rawValue)" just rebuilds the expected value from the same enum case, so inserting/reordering cases before .returnKey can still change the persisted identifier without failing this test. The maxRaw assertion only proves .returnKey is currently last, not that its stored key stayed stable. Lock the current literal (builtin.29) and drop the maxRaw check.
🔧 Suggested test change
let stored = TerminalInputAccessoryAction.returnKey.itemID.storageKey
- `#expect`(stored == "builtin.\(TerminalInputAccessoryAction.returnKey.rawValue)")
+ `#expect`(stored == "builtin.29")
let parsed = ToolbarItemID(storageKey: stored)
`#expect`(parsed == id(.returnKey))
- // Appended last: its raw value is the max across all cases.
- let maxRaw = TerminalInputAccessoryAction.allCases.map(\.rawValue).max()
- `#expect`(TerminalInputAccessoryAction.returnKey.rawValue == maxRaw)Based on Packages/CmuxMobileTerminalKit/Sources/CmuxMobileTerminalKit/ToolbarItemID.swift, the persisted contract is the concrete builtin.<rawValue> string, so the regression test needs to pin that exact value rather than recomputing it from the same enum case.
🤖 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
`@ios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift`
around lines 71 - 83, In the returnKeyStableIdentifier() test function, replace
the dynamic string construction that rebuilds the expected value from the same
enum case with a hardcoded literal string (such as "builtin.29") to ensure the
test catches any unintended changes to the persisted identifier. Additionally,
remove the maxRaw assertion and related lines that only verify the current
ordering rather than the stability of the stored key itself. This ensures the
test will fail if enum reordering accidentally changes the persisted identifier.
| switch self { | ||
| case .escape: return String(localized: "terminal.shortcut.name.escape", defaultValue: "Escape") | ||
| case .tab: return String(localized: "terminal.shortcut.name.tab", defaultValue: "Tab") | ||
| case .returnKey: return String(localized: "terminal.shortcut.name.return", defaultValue: "Return") |
There was a problem hiding this comment.
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
… can read it CI ios-simulator failed: the cmuxFeatureTests module imports CmuxMobileTerminal non-@testable, so the internal output property was inaccessible. title, isUserConfigurable, itemID, and settingsDisplayName are already public; output is the byte payload an action sends, part of the same public contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The touched file is already well over any reasonable size; splitting it is a separate refactor and stored properties cannot move to an extension. Accept the incremental growth as known debt so the budget guard reflects reality.
Autoreview P3: terminal.shortcut.name.return was missing from Localizable.xcstrings while sibling shortcut names have en+ja, so Japanese users would see the English fallback and the localization audit would flag it.
# Conflicts: # .github/swift-file-length-budget.tsv
Adding .returnKey to defaultConfigurableOrder only reaches fresh installs and resets: an upgrading user's persisted v3 config keeps its saved enabled set verbatim, so the appended Return item stayed hidden. Add a one-shot fold (mirroring the existing Shift fold) that inserts and enables Return next to Esc when it is absent from a persisted config, keyed off Return's absence from the saved order so it runs exactly once and a user who later hides it stays hidden. Apply it on the v3, v2, and v1 load paths, generalizing the inline Shift fold into a shared foldNewlyConfigurableV3 helper that chains both folds. Tests: pure chained-fold + idempotence + respect-hidden assertions in the host-testable CmuxMobileTerminalKit migration tests; iOS-layer config tests (ios/cmuxPackage) for the v3 Return fold, the combined Shift+Return fold, later-hide persistence, no re-fold of a hidden Return, one-shot re-persist, and a v2 upgrade surfacing Return. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the P2 autoreview finding: existing users' saved toolbar configs now surface the new Return key. How it works. The layout reducer appends newly-configurable ids to the saved order but preserves the saved enabled set verbatim, so an appended Return stayed hidden after an upgrade. Added a one-shot fold mirroring the existing Shift fold ( Keying. The fold keys off Return's absence from the saved order, not its enabled state. Once it runs, Return is persisted into the v3 order, so every later launch takes the no-op path. A user who then hides Return keeps it hidden across reloads. Generalized the inline Shift fold into a shared Tests.
File-length budget guard green. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift (1)
72-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the persisted Return key to a fixed literal contract value.
Line 78 recomputes the expected key from
rawValue, and Lines 82-83 only assert “is max,” so this test can still pass after raw-value drift. Assert the concrete persisted key literal directly and remove the max-raw check.🔧 Suggested update
let stored = TerminalInputAccessoryAction.returnKey.itemID.storageKey - `#expect`(stored == "builtin.\(TerminalInputAccessoryAction.returnKey.rawValue)") + `#expect`(stored == "builtin.29") // pin concrete persisted contract value let parsed = ToolbarItemID(storageKey: stored) `#expect`(parsed == id(.returnKey)) - // Appended last: its raw value is the max across all cases. - let maxRaw = TerminalInputAccessoryAction.allCases.map(\.rawValue).max() - `#expect`(TerminalInputAccessoryAction.returnKey.rawValue == maxRaw)🤖 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 `@ios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift` around lines 72 - 83, In the returnKeyStableIdentifier() test function, replace the dynamic computation of the storage key using rawValue on line 78 with a hardcoded literal string that represents the fixed contract value for the Return key's persisted identifier. Remove the max-raw assertion at lines 82-83 (the allCases map and max check) since it only validates relative positioning rather than ensuring stability of the actual persisted value. The test should directly assert the concrete literal string value instead of deriving it from the enum's raw value or checking relative orderings.
🤖 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.
Duplicate comments:
In
`@ios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift`:
- Around line 72-83: In the returnKeyStableIdentifier() test function, replace
the dynamic computation of the storage key using rawValue on line 78 with a
hardcoded literal string that represents the fixed contract value for the Return
key's persisted identifier. Remove the max-raw assertion at lines 82-83 (the
allCases map and max check) since it only validates relative positioning rather
than ensuring stability of the actual persisted value. The test should directly
assert the concrete literal string value instead of deriving it from the enum's
raw value or checking relative orderings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8ee2424-51f4-4939-8319-958441ef32e2
📒 Files selected for processing (3)
Packages/CmuxMobileTerminal/Sources/CmuxMobileTerminal/TerminalAccessoryConfiguration.swiftPackages/CmuxMobileTerminalKit/Tests/CmuxMobileTerminalKitTests/ToolbarLayoutMigrationFoldTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/TerminalAccessoryConfigurationTests.swift
This branch was successfully deployed
1 participant
What
Adds a Return/Enter key to the iOS terminal keyboard accessory bar, alongside Tab and Esc. Tapping it sends a carriage return (CR, byte
0x0D), which is what Enter sends in a terminal.Where it sits
In
defaultConfigurableOrderthe new key is placed immediately after Esc (which already sits right after Tab), so the three most common terminal keys (Tab, Esc, Return) are adjacent on a fresh install. It is user-configurable, so it can be hidden/reordered like the other shortcuts.Identifier stability
The new
TerminalInputAccessoryAction.returnKeycase is appended last in the enum. The persisted identifier is the enum'sIntrawValue, stored asbuiltin.<rawValue>, so appending keeps every existing built-in's raw value (and thus every user's persisted bar order/enabled set) unchanged. A UI-test accessibility idterminal.inputAccessory.returnis also added for consistency.Switch sites touched (all in
GhosttySurfaceView.swift)returnKey(appended last)title(isMacRemote:)->⏎accessibilityIdentifier->terminal.inputAccessory.returnoutput->Data([0x0D])(CR)defaultConfigurableOrder-> inserted after.escapesettingsDisplayName-> "Return"Dispatch needs no change: the perform path reads
action.outputgenerically, andisUserConfigurablereturns true by default. All other switches over the enum (icons, alternate/command output, armed/sticky, ResolvedToolbarItem, the configresolve, the reducer/migration) are eitherdefault-covered or enum-agnostic.Tests
Added at the
ios/cmuxPackagelayer (TerminalAccessoryConfigurationTests.swift) because the enum is iOS-only (GhosttySurfaceView.swiftimports UIKit) and is not reachable from host swift-test:0x0D)builtin.<rawValue>, round-trips, raw value is the max across cases)Host-testable reducer tests in
CmuxMobileTerminalKitstill pass (78 tests). iOS compilation is validated by the Xcode build.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized iOS terminal UI and UserDefaults toolbar migration with broad test coverage; no auth, networking, or shared desktop behavior changes.
Overview
Adds a Return/Enter shortcut to the iOS terminal keyboard accessory bar. Tapping ⏎ sends carriage return (
0x0D) through the existingoutputpath; the newreturnKeyenum case is appended last so persistedbuiltin.<rawValue>IDs for other shortcuts stay unchanged.Default layout places Return immediately after Esc (Tab → Esc → Return). It is user-configurable like other shortcuts, with localized settings label and accessibility id
terminal.inputAccessory.return.outputis madepublicfor tests.Migration refactors v3 “fold in post-ship shortcuts” into
foldNewlyConfigurableV3, chaining ⇧ and Return insertion with anchor-based placement. Saved v3, v2→v3, and v1→v3 layouts get Return force-enabled after Esc once (idempotent; respects users who already hid it). Tests cover defaults, persistence, chained folds, and upgrade paths.Reviewed by Cursor Bugbot for commit de3d8d6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a Return/Enter key to the iOS terminal accessory bar next to Tab and Esc. It sends CR (0x0D) and is auto-inserted for existing installs without changing user layouts.
New Features
returnKeyinTerminalInputAccessoryAction; default order Tab → Esc → Return; label "⏎"; accessibility idterminal.inputAccessory.return; sends CR.builtin.<rawValue>ids stay stable;outputis public; settings name localized (en/ja)..github/swift-file-length-budget.tsvto matchGhosttySurfaceView.swift.Migration
Written for commit de3d8d6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation